Fix napi_get_value_string_utf8 to match node (oven-sh#7175)

* fix napi_get_value_string_utf8 to match node closes oven-sh#6949 * [autofix.ci] apply automated fixes * Update test/napi/napi.test.ts Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> * Update test/napi/napi.test.ts Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
ryoppippi · Feb 1, 2024 · 7acbb49 · 7acbb49
1 parent 3678276
commit 7acbb49
Show file tree

Hide file tree

Showing 8 changed files with 187 additions and 4 deletions.
diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp
@@ -1771,15 +1771,22 @@ extern "C" napi_status napi_get_value_string_utf8(napi_env env,
         return napi_ok;
     }
 
-    if (bufsize == NAPI_AUTO_LENGTH) {
-        bufsize = strlen(buf);
+    if (UNLIKELY(bufsize == 0)) {
+        *writtenPtr = 0;
+        return napi_ok;
+    }
+
+    if (UNLIKELY(bufsize == NAPI_AUTO_LENGTH)) {
+        *writtenPtr = 0;
+        buf[0] = '\0';
+        return napi_ok;
     }
 
     size_t written;
     if (view.is8Bit()) {
-        written = Bun__encoding__writeLatin1(view.characters8(), view.length(), reinterpret_cast<unsigned char*>(buf), bufsize, static_cast<uint8_t>(WebCore::BufferEncodingType::utf8));
+        written = Bun__encoding__writeLatin1(view.characters8(), view.length(), reinterpret_cast<unsigned char*>(buf), bufsize - 1, static_cast<uint8_t>(WebCore::BufferEncodingType::utf8));
     } else {
-        written = Bun__encoding__writeUTF16(view.characters16(), view.length(), reinterpret_cast<unsigned char*>(buf), bufsize, static_cast<uint8_t>(WebCore::BufferEncodingType::utf8));
+        written = Bun__encoding__writeUTF16(view.characters16(), view.length(), reinterpret_cast<unsigned char*>(buf), bufsize - 1, static_cast<uint8_t>(WebCore::BufferEncodingType::utf8));
     }
 
     if (writtenPtr != nullptr) {

diff --git a/test/napi/napi-app/.gitignore b/test/napi/napi-app/.gitignore
@@ -0,0 +1,3 @@
+node_modules
+*.log
+build
diff --git a/test/napi/napi-app/binding.gyp b/test/napi/napi-app/binding.gyp
@@ -0,0 +1,18 @@
+{
+    "targets": [{
+        "target_name": "napitests",
+        "cflags!": [ "-fno-exceptions" ],
+        "cflags_cc!": [ "-fno-exceptions" ],
+        "sources": [
+            "main.cpp"
+        ],
+        'include_dirs': [
+            "<!@(node -p \"require('node-addon-api').include\")"
+        ],
+        'libraries': [],
+        'dependencies': [
+            "<!(node -p \"require('node-addon-api').gyp\")"
+        ],
+        'defines': [ 'NAPI_DISABLE_CPP_EXCEPTIONS' ]
+    }]
+}
diff --git a/test/napi/napi-app/bun.lockb b/test/napi/napi-app/bun.lockb
diff --git a/test/napi/napi-app/main.cpp b/test/napi/napi-app/main.cpp
@@ -0,0 +1,67 @@
+/* cppsrc/main.cpp */
+#include <napi.h>
+#include <iostream>
+
+
+
+napi_value fail(napi_env env, const char *msg)
+{
+    napi_value result;
+    napi_create_string_utf8(env, msg, NAPI_AUTO_LENGTH, &result);
+    return result;
+}
+
+napi_value ok(napi_env env)
+{
+    napi_value result;
+    napi_get_undefined(env, &result);
+    return result;
+}
+
+
+napi_value test_napi_get_value_string_utf8_with_buffer(const Napi::CallbackInfo &info)
+{
+    Napi::Env env = info.Env();
+
+    // get how many chars we need to copy
+    uint32_t _len;
+    if (napi_get_value_uint32(env, info[1], &_len) != napi_ok) {
+        return fail(env, "call to napi_get_value_uint32 failed");
+    }
+    size_t len = (size_t)_len;
+
+    if (len == 424242) {
+        len = NAPI_AUTO_LENGTH;
+    } else if (len > 29) {
+        return fail(env, "len > 29");
+    }
+
+    size_t copied;
+    size_t BUF_SIZE = 30;
+    char buf[BUF_SIZE];
+    memset(buf, '*', BUF_SIZE);
+    buf[BUF_SIZE] = '\0';
+
+    if (napi_get_value_string_utf8(env, info[0], buf, len, &copied) != napi_ok) {
+        return fail(env, "call to napi_get_value_string_utf8 failed");
+    }
+
+    std::cout << "Chars to copy: " << len << std::endl;
+    std::cout << "Copied chars: " << copied << std::endl;
+    std::cout << "Buffer: ";
+    for (int i = 0; i < BUF_SIZE; i++) {
+        std::cout << (int)buf[i] << ", ";
+    }
+    std::cout << std::endl;
+    std::cout << "Value str: " << buf << std::endl;
+    return ok(env);
+}
+
+Napi::Object InitAll(Napi::Env env, Napi::Object exports)
+{
+    exports.Set(
+        "test_napi_get_value_string_utf8_with_buffer", Napi::Function::New(env, test_napi_get_value_string_utf8_with_buffer));
+    return exports;
+}
+
+NODE_API_MODULE(napitests, InitAll)
diff --git a/test/napi/napi-app/main.js b/test/napi/napi-app/main.js
@@ -0,0 +1,9 @@
+const tests = require("./build/Release/napitests.node");
+const fn = tests[process.argv[2]];
+if (typeof fn !== "function") {
+  throw new Error("Unknown test:", process.argv[2]);
+}
+const result = fn.apply(null, JSON.parse(process.argv[3] ?? "[]"));
+if (result) {
+  throw new Error(result);
+}
diff --git a/test/napi/napi-app/package.json b/test/napi/napi-app/package.json
@@ -0,0 +1,13 @@
+{
+    "name": "napitests",
+    "version": "1.0.0",
+    "gypfile": true,
+    "scripts": {
+        "build": "node-gyp rebuild",
+        "clean": "node-gyp clean"
+    },
+    "devDependencies": {
+        "node-gyp": "^10.0.1",
+        "node-addon-api": "^7.0.0"
+    }
+}
diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts
@@ -0,0 +1,66 @@
+import { it, expect, test, beforeAll, describe } from "bun:test";
+import { bunExe, bunEnv } from "harness";
+import { spawnSync } from "bun";
+import { join } from "path";
+
+describe("napi", () => {
+  beforeAll(() => {
+    // build gyp
+    const build = spawnSync({
+      cmd: ["yarn", "build"],
+      cwd: join(__dirname, "napi-app"),
+    });
+    if (!build.success) {
+      console.error(build.stderr.toString());
+      throw new Error("build failed");
+    }
+  });
+
+  describe("napi_get_value_string_utf8 with buffer", () => {
+    // see https://github.com/oven-sh/bun/issues/6949
+    it("copies one char", () => {
+      const result = checkSameOutput("test_napi_get_value_string_utf8_with_buffer", ["abcdef", 2]);
+      expect(result).toEndWith("str: a");
+    });
+
+    it("copies null terminator", () => {
+      const result = checkSameOutput("test_napi_get_value_string_utf8_with_buffer", ["abcdef", 1]);
+      expect(result).toEndWith("str:");
+    });
+
+    it("copies zero char", () => {
+      const result = checkSameOutput("test_napi_get_value_string_utf8_with_buffer", ["abcdef", 0]);
+      expect(result).toEndWith("str: ******************************");
+    });
+
+    it("copies more than given len", () => {
+      const result = checkSameOutput("test_napi_get_value_string_utf8_with_buffer", ["abcdef", 25]);
+      expect(result).toEndWith("str: abcdef");
+    });
+
+    it("copies auto len", () => {
+      const result = checkSameOutput("test_napi_get_value_string_utf8_with_buffer", ["abcdef", 424242]);
+      expect(result).toEndWith("str:");
+    });
+  });
+});
+
+function checkSameOutput(test: string, args: any[]) {
+  const nodeResult = runOn("node", test, args).trim();
+  let bunResult = runOn(join(__dirname, "../../build/bun-debug"), test, args);
+  // remove all debug logs
+  bunResult = bunResult.replaceAll(/^\[\w+\].+$/gm, "").trim();
+  expect(bunResult).toBe(nodeResult);
+  return nodeResult;
+}
+
+function runOn(executable: string, test: string, args: any[]) {
+  const exec = spawnSync({
+    cmd: [executable, join(__dirname, "napi-app/main.js"), test, JSON.stringify(args)],
+    env: bunEnv,
+  });
+  const errs = exec.stderr.toString();
+  expect(errs).toBe("");
+  expect(exec.success).toBeTrue();
+  return exec.stdout.toString();
+}