fix(runtime): stop handing freed or foreign buffers to C string parameters

NativeScript/runtime/Interop.mm

@@ -362,24 +362,18 @@ static bool SnapshotArrayLike(Isolate* isolate, Local<Object> object, Local<v8::
     Interop::SetValue(dest, sel_registerName(str.c_str()));
   } else if (typeEncoding->type == BinaryTypeEncodingType::CStringEncoding) {
     if (arg->IsString()) {
-      const char* value = nullptr;
-      Local<v8::String> strArg = arg.As<v8::String>();
-      if (strArg->IsExternalOneByte()) {
-        const v8::String::ExternalOneByteStringResource* resource =
-            strArg->GetExternalOneByteStringResource();
-        value = resource->data();
-      } else {
-        v8::String::Utf8Value utf8Value(isolate, arg);
-        value = strdup(*utf8Value);
-        // The external string only ties the strdup'd buffer's lifetime to the
-        // GC; it is never read as a string, and its one-byte (Latin-1) content
-        // matches the UTF-8 buffer only for ASCII. Length is the buffer's byte
-        // count, excluding the NUL.
-        OneByteStringResource* resource =
-            new OneByteStringResource(value, (size_t)utf8Value.length());
-        bool success = v8::String::NewExternalOneByte(isolate, resource).ToLocal(&arg);
-        tns::Assert(success, isolate);
-      }
+      // Always copy: an external one-byte string's resource (e.g. one made by
+      // node_api_create_external_string_latin1) holds Latin-1 with no NUL
+      // terminator, so its data can't be handed to native code as a C string.
+      v8::String::Utf8Value utf8Value(isolate, arg);
+      const char* value = strdup(*utf8Value);
+      // The external string only ties the strdup'd buffer's lifetime to the
+      // GC; it is never read as a string. Its length counts the NUL so it is
+      // never zero: V8 disposes a zero-length resource immediately.
+      OneByteStringResource* resource =
+          new OneByteStringResource(value, (size_t)utf8Value.length() + 1);
+      bool success = v8::String::NewExternalOneByte(isolate, resource).ToLocal(&arg);
+      tns::Assert(success, isolate);
       Interop::SetValue(dest, value);
     } else {
       BaseDataWrapper* wrapper = tns::GetValue(isolate, arg);

TestFixtures/NapiCoverageModule.mm

@@ -1,7 +1,7 @@
 // Addons see NAPI_VERSION 8 unless they ask for more; asking must come before
 // the headers. The env still reports module API version 8 either way — this
 // only decides which declarations exist.
-#define NAPI_VERSION 9
+#define NAPI_VERSION 10
 
 #include <math.h>
 #include <stdint.h>
@@ -146,6 +146,25 @@ static napi_value CreateStrings(napi_env env, napi_callback_info info) {
   return result;
 }
 
+// The shape addons hand to node_api_create_external_string_latin1: bytes that
+// are Latin-1 (0xE9 is 'é') and not NUL-terminated.
+static napi_value CreateExternalLatin1(napi_env env, napi_callback_info info) {
+  (void)info;
+
+  static char kBytes[] = {'c', 'a', 'f', (char)0xE9, '!'};
+
+  napi_value text = NULL;
+  bool copied = true;
+  NAPI_CALL(env,
+            node_api_create_external_string_latin1(env, kBytes, 4, NULL, NULL, &text, &copied));
+
+  napi_value result = NULL;
+  NAPI_CALL(env, napi_create_object(env, &result));
+  NAPI_CALL(env, napi_set_named_property(env, result, "text", text));
+  NAPI_CALL(env, napi_set_named_property(env, result, "copied", NapiBool(env, copied)));
+  return result;
+}
+
 static napi_value StringStatus(napi_env env, napi_callback_info info) {
   size_t argc = 1;
   napi_value args[1];
@@ -1344,6 +1363,7 @@ static napi_value InitNapiCoverageModule(napi_env env, napi_value exports) {
       NAPI_METHOD("stringLengths", StringLengths),
       NAPI_METHOD("copyString", CopyString),
       NAPI_METHOD("createStrings", CreateStrings),
+      NAPI_METHOD("createExternalLatin1", CreateExternalLatin1),
       NAPI_METHOD("stringStatus", StringStatus),
       NAPI_METHOD("numberParts", NumberParts),
       NAPI_METHOD("createNumbers", CreateNumbers),

TestRunner/app/tests/Marshalling/ReferenceTests.js

@@ -148,6 +148,13 @@ describe(module.id, function () {
         expect(TNSGetOutput()).toBe("abc");
     });
 
+    it("marshals an empty string as an empty C string", function () {
+        TNSClearOutput();
+        var actual = functionWithCharPtr("");
+        expect(TNSGetOutput()).toBe("");
+        expect(actual[0]).toBe(0);
+    });
+
     it("functionWithInt8ArrayBuffer", function () {
         let array = Int8Array.from([ 97, 98, 99, 0 ]).buffer;
         let actual = functionWithCharPtr(array);

TestRunner/app/tests/NapiCoverageTests.js

@@ -46,6 +46,16 @@ describe("Node-API value surface", function () {
             expect(created.empty).toBe("");
         });
 
+        it("hands an external latin1 string to C as UTF-8", function () {
+            var external = napi.createExternalLatin1();
+            expect(external.copied).toBe(false);
+            expect(external.text).toBe("café");
+
+            TNSClearOutput();
+            functionWithCharPtr(external.text);
+            expect(TNSGetOutput()).toBe("café");
+        });
+
         it("honours an explicit length when creating", function () {
             expect(napi.createStrings().utf8Sized).toBe("abc");
         });