From de7f500658b23779418967c8cb28bdfd853f059f Mon Sep 17 00:00:00 2001 From: Zack Radisic <56137411+zackradisic@users.noreply.github.com> Date: Thu, 5 Dec 2024 14:40:42 -0800 Subject: [PATCH] Only set the NapiModuleMeta when it is a bundler plugin and properly check that the input to `napiModule` in JS is indeed a bun native plugihn --- src/bun.js/bindings/BunProcess.cpp | 19 +++++++++++------ src/js/builtins/BundlerPlugin.ts | 32 ++++++++++++++--------------- test/bundler/native-plugin.test.ts | 33 ++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 22 deletions(-) diff --git a/src/bun.js/bindings/BunProcess.cpp b/src/bun.js/bindings/BunProcess.cpp index 628240b254..3026d55433 100644 --- a/src/bun.js/bindings/BunProcess.cpp +++ b/src/bun.js/bindings/BunProcess.cpp @@ -428,12 +428,19 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, EncodedJSValue exportsValue = JSC::JSValue::encode(exports); JSC::JSValue resultValue = JSValue::decode(napi_register_module_v1(globalObject, exportsValue)); - // TODO: think about the finalizer here - // currently we do not dealloc napi modules so we don't have to worry about it right now - auto* meta = new Bun::NapiModuleMeta(globalObject->m_pendingNapiModuleDlopenHandle); - Bun::NapiExternal* napi_external = Bun::NapiExternal::create(vm, globalObject->NapiExternalStructure(), meta, nullptr, nullptr); - bool success = resultValue.getObject()->putDirect(vm, WebCore::builtinNames(vm).napiDlopenHandlePrivateName(), napi_external, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::ReadOnly); - ASSERT(success); + if (auto resultObject = resultValue.getObject()) { + // If this is a native bundler plugin we want to store the handle from dlopen + // as we are going to call `dlsym()` on it later to get the plugin implementation. + const char** pointer_to_plugin_name = (const char**)dlsym(handle, "BUN_PLUGIN_NAME"); + if (pointer_to_plugin_name) { + // TODO: think about the finalizer here + // currently we do not dealloc napi modules so we don't have to worry about it right now + auto* meta = new Bun::NapiModuleMeta(globalObject->m_pendingNapiModuleDlopenHandle); + 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, {}); diff --git a/src/js/builtins/BundlerPlugin.ts b/src/js/builtins/BundlerPlugin.ts index ed2c5e653b..f823435de3 100644 --- a/src/js/builtins/BundlerPlugin.ts +++ b/src/js/builtins/BundlerPlugin.ts @@ -1,11 +1,4 @@ -import type { - BuildConfig, - BunPlugin, - OnLoadCallback, - OnResolveCallback, - PluginBuilder, - PluginConstraints, -} from "bun"; +import type { BuildConfig, BunPlugin, OnLoadCallback, OnResolveCallback, PluginBuilder, PluginConstraints } from "bun"; type AnyFunction = (...args: any[]) => any; interface BundlerPlugin { @@ -73,8 +66,8 @@ export function runSetupFunction( if (map === onBeforeParsePlugins) { isOnBeforeParse = true; // TODO: how to check if it a napi module here? - if (!callback) { - throw new TypeError("onBeforeParse `napiModule` must be a Napi module"); + if (!callback || !$isObject(callback) || !callback.$napiDlopenHandle) { + throw new TypeError("onBeforeParse `napiModule` must be a Napi module which exports the `BUN_PLUGIN_SYMBOL`"); } if (typeof symbol !== "string") { @@ -134,7 +127,7 @@ export function runSetupFunction( const self = this; function onStart(callback) { - if(isBake) { + if (isBake) { throw new TypeError("onStart() is not supported in Bake yet"); } if (!$isCallable(callback)) { @@ -370,7 +363,14 @@ export function runOnResolvePlugins(this: BundlerPlugin, specifier, inputNamespa } } -export function runOnLoadPlugins(this: BundlerPlugin, internalID, path, namespace, defaultLoaderId, isServerSide: boolean) { +export function runOnLoadPlugins( + this: BundlerPlugin, + internalID, + path, + namespace, + defaultLoaderId, + isServerSide: boolean, +) { const LOADERS_MAP = $LoaderLabelToId; const loaderName = $LoaderIdToLabel[defaultLoaderId]; @@ -411,15 +411,15 @@ export function runOnLoadPlugins(this: BundlerPlugin, internalID, path, namespac } var { contents, loader = defaultLoader } = result as any; - if ((loader as any) === 'object') { - if (!('exports' in result)) { + if ((loader as any) === "object") { + if (!("exports" in result)) { throw new TypeError('onLoad plugin returning loader: "object" must have "exports" property'); } try { contents = JSON.stringify(result.exports); - loader = 'json'; + loader = "json"; } catch (e) { - throw new TypeError('When using Bun.build, onLoad plugin must return a JSON-serializable object: ' + e) ; + throw new TypeError("When using Bun.build, onLoad plugin must return a JSON-serializable object: " + e); } } diff --git a/test/bundler/native-plugin.test.ts b/test/bundler/native-plugin.test.ts index 83ef6acaaf..2c1847c774 100644 --- a/test/bundler/native-plugin.test.ts +++ b/test/bundler/native-plugin.test.ts @@ -2,6 +2,7 @@ import { BunFile, Loader, plugin } from "bun"; import { afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test"; import path, { dirname, join, resolve } from "path"; import source from "./native_plugin.cc" with { type: "file" }; +import notAPlugin from "./not_native_plugin.cc" with { type: "file" }; import bundlerPluginHeader from "../../packages/bun-native-bundler-plugin-api/bundler_plugin.h" with { type: "file" }; import { bunEnv, bunExe, tempDirWithFiles } from "harness"; import { itBundled } from "bundler/expectBundled"; @@ -15,6 +16,7 @@ describe("native-plugins", async () => { const files = { "bun-native-bundler-plugin-api/bundler_plugin.h": await Bun.file(bundlerPluginHeader).text(), "plugin.cc": await Bun.file(source).text(), + "not_a_plugin.cc": await Bun.file(notAPlugin).text(), "package.json": JSON.stringify({ "name": "fake-plugin", "module": "index.ts", @@ -48,6 +50,11 @@ values;`, "target_name": "xXx123_foo_counter_321xXx", "sources": [ "plugin.cc" ], "include_dirs": [ "." ] + }, + { + "target_name": "not_a_plugin", + "sources": [ "not_a_plugin.cc" ], + "include_dirs": [ "." ] } ] }`, @@ -491,6 +498,32 @@ const many_foo = ["foo","foo","foo","foo","foo","foo","foo"] expect(compilationCtxFreedCount).toBe(0); }); + it("should fail gracefully when passing something that is NOT a bunler plugin", async () => { + const not_plugins = [require(path.join(tempdir, "build/Release/not_a_plugin.node")), 420, "hi", {}]; + + for (const napiModule of not_plugins) { + try { + await Bun.build({ + outdir, + entrypoints: [path.join(tempdir, "index.ts")], + plugins: [ + { + name: "not_a_plugin", + setup(build) { + build.onBeforeParse({ filter: /\.ts/ }, { napiModule, symbol: "plugin_impl" }); + }, + }, + ], + }); + expect.unreachable(); + } catch (e) { + expect(e.toString()).toContain( + "onBeforeParse `napiModule` must be a Napi module which exports the `BUN_PLUGIN_SYMBOL`", + ); + } + } + }); + it("should use result of the first plugin that runs and doesn't execute the others", async () => { const filter = /\.ts/;