worker-loader: fix potential crash with JSON modules

[git-bruh]

https://jira.cfdata.org/browse/EW-10831

The json field was not actually moved from when passed to serializeJson in worker-loader.c++, causing the dynamic worker script's ownContent to hold the V8Ref pointing to the parent isolate leading to a UaF in the v8::Locker::isLocked call in jsg::Data::destroy during destruction of Module, only if the parent isolate got GC'd before the dynamic isolate

With the fix, the V8Ref's isolate will be set to nullptr, making the jsg::Data::destroy call a no-op

[ask-bonk]

Good, jsg::Optional<T> extends kj::Maybe<T>. So KJ_IF_SOME(json, module.json) binds json as a reference to the inner jsg::Value (which is V8Ref<v8::Value>).

Now the question: after kj::mv(json), the inner V8Ref has isolate = nullptr (from Data's move constructor, line 830). So Data::destroy() on the moved-from value would see isolate == nullptr and skip the v8::Locker::IsLocked call. But the Maybe still holds the moved-from V8Ref (it's not kj::none).

So technically the moved-from V8Ref is already safe (isolate is nulled). But setting module.json = kj::none is still the cleaner/more correct thing to do -- it makes the intent explicit and ensures the moved-from V8Ref doesn't linger. This is defense-in-depth.

Actually wait -- let me re-check. The KJ_IF_SOME macro binds the reference differently depending on the Maybe type. For kj::Maybe<V8Ref<v8::Value>>, it would bind json as V8Ref<v8::Value>&. Then kj::mv(json) is static_cast<V8Ref<v8::Value>&&>(json). The V8Ref move constructor runs, which delegates to Data(Data&& other) on line 826, which sets other.isolate = nullptr.

So after kj::mv(json) on line 226, the V8Ref inside module.json has isolate == nullptr. Its destructor would be a no-op. But module.json = kj::none is still correct to set because:

1. It makes the intent clear to future readers
2. It avoids depending on the internal implementation detail that a moved-from V8Ref is safe to destroy

This is a good defensive fix. The analysis in the comment is accurate about the broader UaF scenario, even if the specific kj::mv already partially mitigates it.

The fix looks correct and well-targeted. The comment is clear and explains the "why". There are no backward compatibility concerns (this is purely an internal ownership fix). No compat flag is needed.

LGTM

github run

[git-bruh]

/bigbonk re-review

[ask-bonk]

Re-review confirms the fix is correct.

The bug: serializeJson(V8Ref<T>&& value) accepted value by rvalue reference but never moved from it. It called value.getHandle(*this) directly, leaving the caller's V8Ref (inside module.json) with a live isolate pointer. When the Module struct outlived the parent isolate and was destroyed, Data::destroy() called v8::Locker::IsLocked(isolate) on freed memory.

The fix: auto moved = kj::mv(value) triggers Data's move constructor, which nulls other.isolate (jsg.h:830). The caller's V8Ref becomes a no-op on destruction. moved is destroyed at end of scope while the isolate lock is still held, which is safe.

Ownership chain verification: At the call site (worker-loader.c++:226), kj::mv(json) binds to the rvalue-ref parameter value. kj::mv(value) then moves from that reference, which is the same object as module.json's inner V8Ref. So the original V8Ref in the Module struct is left with isolate == nullptr. Its Maybe wrapper isn't cleared to kj::none, but that's fine — the moved-from V8Ref destructor is a no-op.

No backward compatibility concerns, no compat flag needed.

---

This review was generated by an AI assistant and may not be perfect.

[ask-bonk]

Re-review posted as an approval on PR #6711. The fix is correct — auto moved = kj::mv(value) properly triggers Data's move constructor which nulls the source's isolate pointer, preventing the UaF in Data::destroy() when the parent isolate is freed before the dynamic worker's Module struct is destroyed.

github run

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.55%. Comparing base (a5a0053) to head (0e78563).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6711      +/-   ##
==========================================
- Coverage   66.55%   66.55%   -0.01%     
==========================================
  Files         402      402              
  Lines      115864   115865       +1     
  Branches    19407    19407              
==========================================
- Hits        77114    77113       -1     
- Misses      27169    27170       +1     
- Partials    11581    11582       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codspeed-hq]

Merging this PR will degrade performance by 28.81%

❌ 1 regressed benchmark
✅ 71 untouched benchmarks
⏩ 129 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
❌	simpleStringBody[Response]	19.2 µs	27 µs	-28.81%

---

_{Comparing pkhanna/fix-worker-loader-uaf (0e78563) with main (a5a0053)}

Footnotes

1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩