Use length to ensure null chars do not cause early termination of C s…

…tring copies/reads (rogchap#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]>
wowngasb · Jan 12, 2022 · 1d433f7 · 1d433f7
1 parent cb8779b
commit 1d433f7
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -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

diff --git a/v8go.cc b/v8go.cc
@@ -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,
@@ -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;
   }
@@ -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) {

diff --git a/v8go.h b/v8go.h
@@ -106,7 +106,8 @@ typedef struct {
 } RtnValue;
 
 typedef struct {
-  const char* string;
+  const char* data;
+  int length;
   RtnError error;
 } RtnString;
 
@@ -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);
@@ -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);

diff --git a/value.go b/value.go
@@ -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
@@ -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{
@@ -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
@@ -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

diff --git a/value_test.go b/value_test.go
@@ -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(){}"},
 	}
@@ -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)