Fix crash when napi_register_module_v1 returns nullptr (#16816)

src/bun.js/bindings/BunProcess.cpp

@@ -438,6 +438,13 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, (JSC::JSGlobalObject * globalOb
 
     EncodedJSValue exportsValue = JSC::JSValue::encode(exports);
     JSC::JSValue resultValue = JSValue::decode(napi_register_module_v1(globalObject, exportsValue));
+    RETURN_IF_EXCEPTION(scope, {});
+    // If a module returns `nullptr` (cast to a napi_value) from its register function, we should
+    // use the `exports` value (which may have had properties added to it) as the return value of
+    // `require()`.
+    if (resultValue.isEmpty()) {
+        resultValue = exports;
+    }
 
     if (auto resultObject = resultValue.getObject()) {
 #if OS(DARWIN) || OS(LINUX)
@@ -454,11 +461,10 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, (JSC::JSGlobalObject * globalOb
             Bun::NapiExternal* napi_external = Bun::NapiExternal::create(vm, globalObject->NapiExternalStructure(), meta, nullptr, nullptr);
             bool success = resultObject->putDirect(vm, WebCore::builtinNames(vm).napiDlopenHandlePrivateName(), napi_external, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::ReadOnly);
             ASSERT(success);
+            RETURN_IF_EXCEPTION(scope, {});
         }
     }
 
-    RETURN_IF_EXCEPTION(scope, {});
-
     globalObject->m_pendingNapiModuleAndExports[0].clear();
     globalObject->m_pendingNapiModuleAndExports[1].clear();
     globalObject->m_pendingNapiModuleDlopenHandle = nullptr;

test/bundler/native_plugin.cc

@@ -559,8 +559,9 @@ napi_value Init(napi_env env, napi_value exports) {
                      "Failed to add create_external function to exports");
     return nullptr;
   }
-
-  return exports;
+  // this should be the same as returning `exports`, but it would crash in
+  // previous versions of Bun
+  return nullptr;
 }
 
 struct NewOnBeforeParseArguments {

test/js/third_party/duckdb/duckdb-basic-usage.test.ts

@@ -0,0 +1,19 @@
+import { libcFamily } from "harness";
+if (libcFamily == "musl") {
+  // duckdb does not distribute musl binaries, so we skip this test on musl to avoid CI noise
+  process.exit(0);
+}
+
+import { describe, test, expect } from "bun:test";
+// Must be CJS require so that the above code can exit before we attempt to import DuckDB
+const { Database } = require("duckdb");
+
+describe("duckdb", () => {
+  test("basic usage", () => {
+    const db = new Database(":memory:");
+    db.all("SELECT 42 AS fortytwo", (err, res) => {
+      expect(err).toBeNull();
+      expect(res[0].fortytwo).toBe(42);
+    });
+  });
+});

test/napi/napi-app/binding.gyp

@@ -30,5 +30,53 @@
                 "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
             ],
         },
+        {
+            "target_name": "nullptr_addon",
+            "sources": ["null_addon.cpp"],
+            "include_dirs": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+                "MODULE_INIT_RETURN_NULLPTR=1"
+            ],
+        },
+        {
+            "target_name": "null_addon",
+            "sources": ["null_addon.cpp"],
+            "include_dirs": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+                "MODULE_INIT_RETURN_NULL=1"
+            ],
+        },
+        {
+            "target_name": "undefined_addon",
+            "sources": ["null_addon.cpp"],
+            "include_dirs": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+                "MODULE_INIT_RETURN_UNDEFINED=1"
+            ],
+        },
+        {
+            "target_name": "throw_addon",
+            "sources": ["null_addon.cpp"],
+            "include_dirs": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+                "MODULE_INIT_THROW=1"
+            ],
+        },
     ]
 }

test/napi/napi-app/null_addon.cpp

@@ -0,0 +1,48 @@
+#include <js_native_api.h>
+#include <node_api.h>
+
+#define NODE_API_CALL(env, call)                                               \
+  do {                                                                         \
+    napi_status status = (call);                                               \
+    if (status != napi_ok) {                                                   \
+      const napi_extended_error_info *error_info = NULL;                       \
+      napi_get_last_error_info((env), &error_info);                            \
+      const char *err_message = error_info->error_message;                     \
+      bool is_pending;                                                         \
+      napi_is_exception_pending((env), &is_pending);                           \
+      /* If an exception is already pending, don't rethrow it */               \
+      if (!is_pending) {                                                       \
+        const char *message =                                                  \
+            (err_message == NULL) ? "empty error message" : err_message;       \
+        napi_throw_error((env), NULL, message);                                \
+      }                                                                        \
+      return NULL;                                                             \
+    }                                                                          \
+  } while (0)
+
+/* napi_value */ NAPI_MODULE_INIT(/* napi_env env, napi_value exports */) {
+  napi_value number;
+  NODE_API_CALL(env, napi_create_int32(env, 123, &number));
+  NODE_API_CALL(env, napi_set_named_property(env, exports, "number", number));
+
+  // These defines are used by binding.gyp to compile three versions of this
+  // module that return different values
+#if defined(MODULE_INIT_RETURN_NULLPTR)
+  // returning NULL means the exports value should be used as the return value
+  // of require()
+  return NULL;
+#elif defined(MODULE_INIT_RETURN_NULL)
+  napi_value null;
+  NODE_API_CALL(env, napi_get_null(env, &null));
+  return null;
+#elif defined(MODULE_INIT_RETURN_UNDEFINED)
+  napi_value undefined;
+  NODE_API_CALL(env, napi_get_undefined(env, &undefined));
+  return undefined;
+#elif defined(MODULE_INIT_THROW)
+  napi_throw_error(env, "CODE_OOPS", "oops!");
+  return NULL;
+#else
+#error Define one of MODULE_INIT_RETURN_{NULLPTR,NULL,UNDEFINED} to determine what to return from NAPI_MODULE_INIT
+#endif
+}

test/napi/napi.test.ts

@@ -383,6 +383,17 @@ describe("napi", () => {
       checkSameOutput("test_extended_error_messages", []);
     });
   });
+
+  it.each([
+    ["nullptr", { number: 123 }],
+    ["null", null],
+    ["undefined", undefined],
+  ])("works when the module register function returns %s", (returnKind, expected) => {
+    expect(require(`./napi-app/build/Release/${returnKind}_addon.node`)).toEqual(expected);
+  });
+  it("works when the module register function throws", () => {
+    expect(() => require("./napi-app/build/Release/throw_addon.node")).toThrow(new Error("oops!"));
+  });
 });
 
 function checkSameOutput(test: string, args: any[] | string) {

test/package.json

@@ -29,6 +29,7 @@
     "body-parser": "1.20.2",
     "comlink": "4.4.1",
     "devalue": "5.1.1",
+    "duckdb": "1.1.3",
     "es-module-lexer": "1.3.0",
     "esbuild": "0.18.6",
     "express": "4.18.2",