From 71d85286c05b0e0313e41925ae169d5a2b64fffa Mon Sep 17 00:00:00 2001 From: Beau Collins Date: Wed, 19 Aug 2026 11:03:19 -0700 Subject: [PATCH 1/3] Retain trace-proxy callback (fix Hermes use-after-free) JavaScriptRuntimeTraceProxyCallable held the wrapped callback as a bare, non-owning JSValue. Under Hermes the callback's pooled ManagedHermesValue could be released to zero and its slot recycled while the proxy still referenced it, so a later invocation hit a dangling handle and threw "Value is not a function". Hold it as an owning JSValueRef instead, retained for the proxy's lifetime. Co-Authored-By: Claude Opus 4.8 --- valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp b/valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp index 5f0b468ce..720071682 100644 --- a/valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp +++ b/valdi/src/valdi/runtime/JavaScript/JavaScriptRuntime.cpp @@ -176,7 +176,7 @@ class JavaScriptRuntimeTraceProxyCallable : public JSFunction { const JSValue& callback) : _traceName(traceName), _referenceInfo(ReferenceInfoBuilder().withObject(nameFromJSFunction(jsContext, callback)).build()), - _callback(callback) {} + _callback(JSValueRef::makeRetained(jsContext, callback)) {} ~JavaScriptRuntimeTraceProxyCallable() override = default; @@ -186,13 +186,13 @@ class JavaScriptRuntimeTraceProxyCallable : public JSFunction { JSValueRef operator()(JSFunctionNativeCallContext& callContext) noexcept override { VALDI_TRACE(_traceName); - return callContext.getContext().callObjectAsFunction(_callback, callContext); + return callContext.getContext().callObjectAsFunction(_callback.get(), callContext); } private: StringBox _traceName; ReferenceInfo _referenceInfo; - JSValue _callback; + JSValueRef _callback; }; class JSRuntimeNativeObjectsManagerImpl : public snap::valdi_core::JSRuntimeNativeObjectsManager { From 2f05adac54a933b9ed09af11f4eb4a1429f92992 Mon Sep 17 00:00:00 2001 From: Beau Collins Date: Wed, 19 Aug 2026 11:09:56 -0700 Subject: [PATCH 2/3] Add regression test for trace-proxy callback retention Wraps an inline closure in runtime.makeTraceProxy, forces GC, then invokes the proxy. Pre-fix under Hermes the unrooted closure was collected and its slot recycled, so the call threw 'Value is not a function'. Runs across engines via the parametrized RuntimeFixture. Co-Authored-By: Claude Opus 4.8 --- valdi/test/integration/Runtime_tests.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/valdi/test/integration/Runtime_tests.cpp b/valdi/test/integration/Runtime_tests.cpp index dafad6825..aef66adcc 100644 --- a/valdi/test/integration/Runtime_tests.cpp +++ b/valdi/test/integration/Runtime_tests.cpp @@ -272,6 +272,30 @@ TEST_P(RuntimeFixture, exposesDefaultApiVersion) { ASSERT_EQ(0, evalResult.value().toInt()); } +TEST_P(RuntimeFixture, traceProxyRetainsCallbackAcrossGC) { + // Regression: JavaScriptRuntimeTraceProxyCallable used to hold its wrapped callback as a bare, + // non-owning JSValue. Under Hermes the callback's pooled value could be collected and its slot + // recycled while the proxy still referenced it, so invoking the proxy hit a dangling handle and + // threw "Value is not a function". The proxy must retain the callback for its own lifetime. + auto javaScriptRuntime = wrapper.runtime->getJavaScriptRuntime(); + + // The closure has no JS reference other than the trace proxy itself. + std::string setupBody = "globalThis.__traceProxy = runtime.makeTraceProxy('regressionTag', () => 42); return 0;"; + auto setupResult = + javaScriptRuntime->evaluateScript(makeShared(setupBody)->toBytesView(), STRING_LITERAL("eval.js")); + ASSERT_TRUE(setupResult) << setupResult.description(); + + // Force GC: pre-fix the unrooted closure is collected and its slot recycled. + javaScriptRuntime->dispatchSynchronouslyOnJsThread([](auto& jsEntry) { jsEntry.jsContext.garbageCollect(); }); + + // Invoke through the proxy: it must still resolve to the original callback. + std::string callBody = "return __traceProxy();"; + auto callResult = + javaScriptRuntime->evaluateScript(makeShared(callBody)->toBytesView(), STRING_LITERAL("eval.js")); + ASSERT_TRUE(callResult) << callResult.description(); + ASSERT_EQ(42, callResult.value().toInt()); +} + TEST_P(RuntimeFixture, canLoadSimpleViewTree) { auto tree = wrapper.createViewNodeTreeAndContext("test", "BasicViewTree"); From 8667e3af99c84d023bd3a6cf77fbc1a68d3243ea Mon Sep 17 00:00:00 2001 From: Beau Collins Date: Wed, 19 Aug 2026 14:28:31 -0700 Subject: [PATCH 3/3] Skip trace-proxy regression test when tracing is compiled out runtime.makeTraceProxy is only bound under kTracingEnabled, so the test can only exercise the bug in a tracing build. Probe for the binding and GTEST_SKIP when it's absent instead of failing. Co-Authored-By: Claude Opus 4.8 --- valdi/test/integration/Runtime_tests.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/valdi/test/integration/Runtime_tests.cpp b/valdi/test/integration/Runtime_tests.cpp index aef66adcc..36e81b4b0 100644 --- a/valdi/test/integration/Runtime_tests.cpp +++ b/valdi/test/integration/Runtime_tests.cpp @@ -279,6 +279,16 @@ TEST_P(RuntimeFixture, traceProxyRetainsCallbackAcrossGC) { // threw "Value is not a function". The proxy must retain the callback for its own lifetime. auto javaScriptRuntime = wrapper.runtime->getJavaScriptRuntime(); + // makeTraceProxy is only bound when tracing is compiled in (kTracingEnabled); skip otherwise. + auto probe = javaScriptRuntime->evaluateScript( + makeShared(std::string("return typeof runtime.makeTraceProxy === 'function' ? 1 : 0;")) + ->toBytesView(), + STRING_LITERAL("eval.js")); + ASSERT_TRUE(probe) << probe.description(); + if (probe.value().toInt() != 1) { + GTEST_SKIP() << "tracing disabled in this build (runtime.makeTraceProxy not bound)"; + } + // The closure has no JS reference other than the trace proxy itself. std::string setupBody = "globalThis.__traceProxy = runtime.makeTraceProxy('regressionTag', () => 42); return 0;"; auto setupResult =