Skip to content

Commit

Permalink
Use length to ensure null chars do not cause early termination of C s…
Browse files Browse the repository at this point in the history
…tring copies/reads (#272)

* Intro test case for null terminator string

* unexpected result: expected ss, got sx00s

* Fix the assertion, add Unicode symbols

* Pass go string length into v8::String::New to ensure it does not cut off
at null chars

* Reuse the existing RtnString type

* Formatting

* Update value_test.go

Co-authored-by: Dylan Thacker-Smith <[email protected]>

* Table tests for NewValue

* Use std::string constructor in CopyString(Utf8Value) to keep whole
string

* Update changelog

Co-authored-by: Genevieve L'Esperance <[email protected]>
Co-authored-by: Maxim Shirshin <[email protected]>
Co-authored-by: Dylan Thacker-Smith <[email protected]>
  • Loading branch information
4 people authored Jan 12, 2022
1 parent cb8779b commit 1d433f7
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 18 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed
- Use string length to ensure null character-containing strings in Go/JS are not terminated early.

## [v0.7.0] - 2021-12-09

### Added
Expand Down
21 changes: 14 additions & 7 deletions v8go.cc
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ const char* CopyString(String::Utf8Value& value) {
if (value.length() == 0) {
return nullptr;
}
return CopyString(*value);
return CopyString(std::string(*value, value.length()));
}

static RtnError ExceptionError(TryCatch& try_catch,
Expand Down Expand Up @@ -808,12 +808,13 @@ ValuePtr NewValueIntegerFromUnsigned(IsolatePtr iso, uint32_t v) {
return tracked_value(ctx, val);
}

RtnValue NewValueString(IsolatePtr iso, const char* v) {
RtnValue NewValueString(IsolatePtr iso, const char* v, int v_length) {
ISOLATE_SCOPE_INTERNAL_CONTEXT(iso);
TryCatch try_catch(iso);
RtnValue rtn = {};
Local<String> str;
if (!String::NewFromUtf8(iso, v).ToLocal(&str)) {
if (!String::NewFromUtf8(iso, v, NewStringType::kNormal, v_length)
.ToLocal(&str)) {
rtn.error = ExceptionError(try_catch, iso, ctx->ptr.Get(iso));
return rtn;
}
Expand Down Expand Up @@ -948,18 +949,24 @@ RtnString ValueToDetailString(ValuePtr ptr) {
return rtn;
}
String::Utf8Value ds(iso, str);
rtn.string = CopyString(ds);
rtn.data = CopyString(ds);
rtn.length = ds.length();
return rtn;
}

const char* ValueToString(ValuePtr ptr) {
RtnString ValueToString(ValuePtr ptr) {
LOCAL_VALUE(ptr);
RtnString rtn = {0};
// String::Utf8Value will result in an empty string if conversion to a string
// fails
// TODO: Consider propagating the JS error. A fallback value could be returned
// in Value.String()
String::Utf8Value utf8(iso, value);
return CopyString(utf8);
String::Utf8Value src(iso, value);
char* data = static_cast<char*>(malloc(src.length()));
memcpy(data, *src, src.length());
rtn.data = data;
rtn.length = src.length();
return rtn;
}

uint32_t ValueToUint32(ValuePtr ptr) {
Expand Down
7 changes: 4 additions & 3 deletions v8go.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ typedef struct {
} RtnValue;

typedef struct {
const char* string;
const char* data;
int length;
RtnError error;
} RtnString;

Expand Down Expand Up @@ -193,7 +194,7 @@ extern ValuePtr NewValueNull(IsolatePtr iso_ptr);
extern ValuePtr NewValueUndefined(IsolatePtr iso_ptr);
extern ValuePtr NewValueInteger(IsolatePtr iso_ptr, int32_t v);
extern ValuePtr NewValueIntegerFromUnsigned(IsolatePtr iso_ptr, uint32_t v);
extern RtnValue NewValueString(IsolatePtr iso_ptr, const char* v);
extern RtnValue NewValueString(IsolatePtr iso_ptr, const char* v, int v_length);
extern ValuePtr NewValueBoolean(IsolatePtr iso_ptr, int v);
extern ValuePtr NewValueNumber(IsolatePtr iso_ptr, double v);
extern ValuePtr NewValueBigInt(IsolatePtr iso_ptr, int64_t v);
Expand All @@ -202,7 +203,7 @@ extern RtnValue NewValueBigIntFromWords(IsolatePtr iso_ptr,
int sign_bit,
int word_count,
const uint64_t* words);
const char* ValueToString(ValuePtr ptr);
extern RtnString ValueToString(ValuePtr ptr);
const uint32_t* ValueToArrayIndex(ValuePtr ptr);
int ValueToBoolean(ValuePtr ptr);
int32_t ValueToInt32(ValuePtr ptr);
Expand Down
14 changes: 6 additions & 8 deletions value.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ func Null(iso *Isolate) *Value {
// string -> V8::String
// int32 -> V8::Integer
// uint32 -> V8::Integer
// bool -> V8::Boolean
// int64 -> V8::BigInt
// uint64 -> V8::BigInt
// bool -> V8::Boolean
Expand All @@ -73,7 +72,7 @@ func NewValue(iso *Isolate, val interface{}) (*Value, error) {
case string:
cstr := C.CString(v)
defer C.free(unsafe.Pointer(cstr))
rtn := C.NewValueString(iso.ptr, cstr)
rtn := C.NewValueString(iso.ptr, cstr, C.int(len(v)))
return valueResult(nil, rtn)
case int32:
rtnVal = &Value{
Expand Down Expand Up @@ -199,13 +198,12 @@ func (v *Value) Boolean() bool {
// DetailString provide a string representation of this value usable for debugging.
func (v *Value) DetailString() string {
rtn := C.ValueToDetailString(v.ptr)
if rtn.string == nil {
if rtn.data == nil {
err := newJSError(rtn.error)
panic(err) // TODO: Return a fallback value
}
s := rtn.string
defer C.free(unsafe.Pointer(s))
return C.GoString(s)
defer C.free(unsafe.Pointer(rtn.data))
return C.GoStringN(rtn.data, rtn.length)
}

// Int32 perform the equivalent of `Number(value)` in JS and convert the result to a
Expand Down Expand Up @@ -242,8 +240,8 @@ func (v *Value) Object() *Object {
// print their definition.
func (v *Value) String() string {
s := C.ValueToString(v.ptr)
defer C.free(unsafe.Pointer(s))
return C.GoString(s)
defer C.free(unsafe.Pointer(s.data))
return C.GoStringN(s.data, C.int(s.length))
}

// Uint32 perform the equivalent of `Number(value)` in JS and convert the result to an
Expand Down
46 changes: 46 additions & 0 deletions value_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ func TestValueString(t *testing.T) {
}{
{"Number", `13 * 2`, "26"},
{"String", `"string"`, "string"},
{"String with null character and non-latin unicode", `"a\x00Ω"`, "a\x00Ω"},
{"Object", `let obj = {}; obj`, "[object Object]"},
{"Function", `let fn = function(){}; fn`, "function(){}"},
}
Expand All @@ -97,6 +98,51 @@ func TestValueString(t *testing.T) {
}
}

func TestNewValue(t *testing.T) {
t.Parallel()
ctx := v8.NewContext(nil)
iso := ctx.Isolate()
defer iso.Dispose()
defer ctx.Close()

tests := []struct {
name string
input interface{}
predicate string
}{
{"string", "s\x00s\x00", `str => str === "s\x00s\x00"`},
{"int32", int32(36), `int => int === 36`},
{"bool", true, `b => b === true`},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
val, err := ctx.RunScript(tt.predicate, "test.js")
if err != nil {
t.Fatal(err)
}
fn, err := val.AsFunction()
if err != nil {
t.Fatal(err)
}

jsVal, err := v8.NewValue(iso, tt.input)
if err != nil {
t.Fatal(err)
}

result, err := fn.Call(ctx.Global(), jsVal)
if err != nil {
t.Fatal(err)
}
if !result.Boolean() {
t.Fatal("unexpected result: expected true, got false")
}
})
}
}

func TestValueDetailString(t *testing.T) {
t.Parallel()
ctx := v8.NewContext(nil)
Expand Down

0 comments on commit 1d433f7

Please sign in to comment.