From 1611ced9d6a1dbe27d7994aefd1b46c8460def7a Mon Sep 17 00:00:00 2001 From: Genevieve L'Esperance Date: Thu, 6 Jan 2022 13:24:42 -0800 Subject: [PATCH 01/10] Intro test case for null terminator string --- value_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/value_test.go b/value_test.go index 4b2c019c..820aeb7f 100644 --- 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", "String.fromCharCode(115) + String.fromCharCode(0) + String.fromCharCode(115)", "ss"}, {"Object", `let obj = {}; obj`, "[object Object]"}, {"Function", `let fn = function(){}; fn`, "function(){}"}, } From 1911ffab204854ada5198cf918ed13c0434ae9d2 Mon Sep 17 00:00:00 2001 From: Genevieve L'Esperance Date: Thu, 6 Jan 2022 14:20:16 -0800 Subject: [PATCH 02/10] unexpected result: expected ss, got sx00s --- v8go.cc | 8 +++++--- v8go.h | 7 ++++++- value.go | 4 ++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/v8go.cc b/v8go.cc index 7c682d0d..f071b93a 100644 --- a/v8go.cc +++ b/v8go.cc @@ -952,14 +952,16 @@ RtnString ValueToDetailString(ValuePtr ptr) { return rtn; } -const char* ValueToString(ValuePtr ptr) { +FullString ValueToString(ValuePtr ptr) { LOCAL_VALUE(ptr); // 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(malloc(src.length())); + memcpy(data, *src, src.length()); + return (FullString){data, src.length()}; } uint32_t ValueToUint32(ValuePtr ptr) { diff --git a/v8go.h b/v8go.h index 051dc558..9f3af5e6 100644 --- a/v8go.h +++ b/v8go.h @@ -110,6 +110,11 @@ typedef struct { RtnError error; } RtnString; +typedef struct { + const char* data; + int length; +} FullString; + typedef struct { size_t total_heap_size; size_t total_heap_size_executable; @@ -202,7 +207,7 @@ extern RtnValue NewValueBigIntFromWords(IsolatePtr iso_ptr, int sign_bit, int word_count, const uint64_t* words); -const char* ValueToString(ValuePtr ptr); +extern FullString 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 index dcfc0ab1..7be9eaa0 100644 --- a/value.go +++ b/value.go @@ -242,8 +242,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 From b7bbe795671ecfc2f0575ada64f016624aec33af Mon Sep 17 00:00:00 2001 From: Maxim Shirshin Date: Fri, 7 Jan 2022 11:01:46 +0100 Subject: [PATCH 03/10] Fix the assertion, add Unicode symbols --- value_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/value_test.go b/value_test.go index 820aeb7f..613611d0 100644 --- a/value_test.go +++ b/value_test.go @@ -81,7 +81,7 @@ func TestValueString(t *testing.T) { }{ {"Number", `13 * 2`, "26"}, {"String", `"string"`, "string"}, - {"String with null", "String.fromCharCode(115) + String.fromCharCode(0) + String.fromCharCode(115)", "ss"}, + {"String with null terminators and Unicode", "String.fromCharCode(0, 55358, 56614, 8205, 9794, 65039, 0, 1040, 1041, 1042, 0, 97, 98, 99, 100)", "\x00\U0001f926\u200d\u2642\ufe0f\x00АБВ\x00abcd"}, {"Object", `let obj = {}; obj`, "[object Object]"}, {"Function", `let fn = function(){}; fn`, "function(){}"}, } From 3348d80946d602401b00a66fbb05b16df8b47e50 Mon Sep 17 00:00:00 2001 From: Genevieve L'Esperance Date: Fri, 7 Jan 2022 09:08:00 -0800 Subject: [PATCH 04/10] Pass go string length into v8::String::New to ensure it does not cut off at null chars --- v8go.cc | 4 ++-- v8go.h | 2 +- value.go | 2 +- value_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/v8go.cc b/v8go.cc index f071b93a..c01852a3 100644 --- a/v8go.cc +++ b/v8go.cc @@ -808,12 +808,12 @@ 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 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; } diff --git a/v8go.h b/v8go.h index 9f3af5e6..22925968 100644 --- a/v8go.h +++ b/v8go.h @@ -198,7 +198,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); diff --git a/value.go b/value.go index 7be9eaa0..e47a373f 100644 --- a/value.go +++ b/value.go @@ -73,7 +73,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{ diff --git a/value_test.go b/value_test.go index 613611d0..f235dfed 100644 --- a/value_test.go +++ b/value_test.go @@ -98,6 +98,44 @@ func TestValueString(t *testing.T) { } } +func TestValueString_GoToJSAndBack(t *testing.T) { + t.Parallel() + ctx := v8.NewContext(nil) + iso := ctx.Isolate() + defer iso.Dispose() + defer ctx.Close() + + str := "s\x00s\x00" + jsStr, err := v8.NewValue(iso, str) + if err != nil { + t.Fatal(err) + } + + // Test whether the go->js keeps the null chars + val, err := ctx.RunScript("(str) => { return str === String.fromCharCode(115, 0, 115, 0)}", "test.js") + if err != nil { + t.Fatal(err) + } + fn, err := val.AsFunction() + if err != nil { + t.Fatal(err) + } + + result, err := fn.Call(ctx.Global(), jsStr) + if err != nil { + t.Fatal(err) + } + if !result.Boolean() { + t.Fatal("unexpected result: expected true, got false") + } + + // Test whether the js->go keeps the null chars + goStr := jsStr.String() + if goStr != str { + t.Errorf("unexpected result: expected %q, got %q", str, goStr) + } +} + func TestValueDetailString(t *testing.T) { t.Parallel() ctx := v8.NewContext(nil) From 8eb85504e43ab4fc5a420ab4552776e934681b13 Mon Sep 17 00:00:00 2001 From: Genevieve L'Esperance Date: Fri, 7 Jan 2022 09:27:37 -0800 Subject: [PATCH 05/10] Reuse the existing RtnString type --- v8go.cc | 9 ++++++--- v8go.h | 10 +++------- value.go | 4 ++-- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/v8go.cc b/v8go.cc index c01852a3..26dfe01d 100644 --- a/v8go.cc +++ b/v8go.cc @@ -948,12 +948,13 @@ RtnString ValueToDetailString(ValuePtr ptr) { return rtn; } String::Utf8Value ds(iso, str); - rtn.string = CopyString(ds); + rtn.data = CopyString(ds); return rtn; } -FullString 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 @@ -961,7 +962,9 @@ FullString ValueToString(ValuePtr ptr) { String::Utf8Value src(iso, value); char* data = static_cast(malloc(src.length())); memcpy(data, *src, src.length()); - return (FullString){data, src.length()}; + rtn.data = data; + rtn.length = src.length(); + return rtn; } uint32_t ValueToUint32(ValuePtr ptr) { diff --git a/v8go.h b/v8go.h index 22925968..7acaf042 100644 --- a/v8go.h +++ b/v8go.h @@ -105,15 +105,11 @@ typedef struct { RtnError error; } RtnValue; -typedef struct { - const char* string; - RtnError error; -} RtnString; - typedef struct { const char* data; int length; -} FullString; + RtnError error; +} RtnString; typedef struct { size_t total_heap_size; @@ -207,7 +203,7 @@ extern RtnValue NewValueBigIntFromWords(IsolatePtr iso_ptr, int sign_bit, int word_count, const uint64_t* words); -extern FullString 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 index e47a373f..5c4f14bb 100644 --- a/value.go +++ b/value.go @@ -199,11 +199,11 @@ 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 + s := rtn.data defer C.free(unsafe.Pointer(s)) return C.GoString(s) } From 6009677c53b4cc35a0229bac79da9ac799f34464 Mon Sep 17 00:00:00 2001 From: Genevieve L'Esperance Date: Tue, 11 Jan 2022 08:47:45 -0800 Subject: [PATCH 06/10] Formatting --- v8go.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/v8go.cc b/v8go.cc index 26dfe01d..b17c8566 100644 --- a/v8go.cc +++ b/v8go.cc @@ -813,7 +813,8 @@ RtnValue NewValueString(IsolatePtr iso, const char* v, int v_length) { TryCatch try_catch(iso); RtnValue rtn = {}; Local str; - if (!String::NewFromUtf8(iso, v, NewStringType::kNormal, v_length).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; } From f6accef688ae3d4cf0c43ae3a8ac2db6a89effeb Mon Sep 17 00:00:00 2001 From: Genevieve Date: Tue, 11 Jan 2022 13:24:02 -0800 Subject: [PATCH 07/10] Update value_test.go Co-authored-by: Dylan Thacker-Smith --- value_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/value_test.go b/value_test.go index f235dfed..073d8957 100644 --- a/value_test.go +++ b/value_test.go @@ -81,7 +81,7 @@ func TestValueString(t *testing.T) { }{ {"Number", `13 * 2`, "26"}, {"String", `"string"`, "string"}, - {"String with null terminators and Unicode", "String.fromCharCode(0, 55358, 56614, 8205, 9794, 65039, 0, 1040, 1041, 1042, 0, 97, 98, 99, 100)", "\x00\U0001f926\u200d\u2642\ufe0f\x00АБВ\x00abcd"}, + {"String with null character and non-latin unicode", `"a\x00Ω"`, "a\x00Ω"}, {"Object", `let obj = {}; obj`, "[object Object]"}, {"Function", `let fn = function(){}; fn`, "function(){}"}, } From 1942901c72fa7d660ff2dbdddb026313dbdfac24 Mon Sep 17 00:00:00 2001 From: Genevieve L'Esperance Date: Tue, 11 Jan 2022 13:44:46 -0800 Subject: [PATCH 08/10] Table tests for NewValue --- v8go.cc | 1 + value.go | 6 ++---- value_test.go | 57 +++++++++++++++++++++++++++++---------------------- 3 files changed, 35 insertions(+), 29 deletions(-) diff --git a/v8go.cc b/v8go.cc index b17c8566..efcd7ed4 100644 --- a/v8go.cc +++ b/v8go.cc @@ -950,6 +950,7 @@ RtnString ValueToDetailString(ValuePtr ptr) { } String::Utf8Value ds(iso, str); rtn.data = CopyString(ds); + rtn.length = ds.length(); return rtn; } diff --git a/value.go b/value.go index 5c4f14bb..c4ba9acf 100644 --- 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 @@ -203,9 +202,8 @@ func (v *Value) DetailString() string { err := newJSError(rtn.error) panic(err) // TODO: Return a fallback value } - s := rtn.data - 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 diff --git a/value_test.go b/value_test.go index 073d8957..d386a589 100644 --- a/value_test.go +++ b/value_test.go @@ -98,41 +98,48 @@ func TestValueString(t *testing.T) { } } -func TestValueString_GoToJSAndBack(t *testing.T) { +func TestNewValue(t *testing.T) { t.Parallel() ctx := v8.NewContext(nil) iso := ctx.Isolate() defer iso.Dispose() defer ctx.Close() - str := "s\x00s\x00" - jsStr, err := v8.NewValue(iso, str) - if err != nil { - t.Fatal(err) + 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`}, } - // Test whether the go->js keeps the null chars - val, err := ctx.RunScript("(str) => { return str === String.fromCharCode(115, 0, 115, 0)}", "test.js") - if err != nil { - t.Fatal(err) - } - fn, err := val.AsFunction() - if err != nil { - t.Fatal(err) - } + 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) + } - result, err := fn.Call(ctx.Global(), jsStr) - if err != nil { - t.Fatal(err) - } - if !result.Boolean() { - t.Fatal("unexpected result: expected true, got false") - } + jsVal, err := v8.NewValue(iso, tt.input) + if err != nil { + t.Fatal(err) + } - // Test whether the js->go keeps the null chars - goStr := jsStr.String() - if goStr != str { - t.Errorf("unexpected result: expected %q, got %q", str, goStr) + result, err := fn.Call(ctx.Global(), jsVal) + if err != nil { + t.Fatal(err) + } + if !result.Boolean() { + t.Fatal("unexpected result: expected true, got false") + } + }) } } From 396ce4dfe1e97d267dae51a4f56181535ea2c73e Mon Sep 17 00:00:00 2001 From: Genevieve L'Esperance Date: Wed, 12 Jan 2022 09:00:19 -0800 Subject: [PATCH 09/10] Use std::string constructor in CopyString(Utf8Value) to keep whole string --- v8go.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/v8go.cc b/v8go.cc index a3127cca..97390883 100644 --- 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, From 1361f8ea960f83974c826aeeaef4e60f24582b56 Mon Sep 17 00:00:00 2001 From: Genevieve L'Esperance Date: Wed, 12 Jan 2022 09:32:26 -0800 Subject: [PATCH 10/10] Update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23dab97d..aeec892f 100644 --- 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