ReviewOS

zig-utils/zig-js

Function constructors run code injected through the body string

#946
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Found in the post-#933 survey and executed against a Debug build of main (f2bafeb), with node v24.4.1 as the oracle.

new Function(P, body) (and the GeneratorFunction, AsyncFunction and AsyncGeneratorFunction constructors) parse the parameters on their own. The body is only ever parsed inside the assembled (function anonymous(P\n) {\nbody\n}). A body that closes the function early therefore passes, and whatever follows it runs when the function is constructed:

programnodezig-js
var r; new Function("}); r = \"injected\"; (function(){"); rSyntaxErrorinjected
the same body through GeneratorFunction and AsyncFunctionSyntaxErrorinjected
new Function("}, function(){ return \"inj\" ")()SyntaxErrorinj (the constructor returns the injected function)
bodies }, function(){ and })(); (function(){SyntaxErroraccepted

CreateDynamicFunction (ECMA-262 20.2.1.1.1) requires the body text to parse on its own as a FunctionBody before the assembled source is parsed. V8 and JavaScriptCore implement the same rule by checking that the single parse yields exactly one function literal ending at the appended }.

This is both a conformance bug and an embedder-safety one: code that builds a function from an untrusted body string expects a SyntaxError, not execution.

Fix: after parsing the assembled source, require exactly one expression statement that is a function expression whose body closes at the constructor's own }, in both constructor paths.

Done: all six programs throw SyntaxError in all four constructors. Valid bodies are unchanged, including a trailing // comment and a leading HTML close comment -->. Pinned as focused cases. Debug before/after --diag over test/built-ins/Function, GeneratorFunction, AsyncFunction, AsyncGeneratorFunction and test/annexB shows 0 failures on both sides.

10 days ago

Taking this: the fix is written and its before/after gate (probe corpus, Debug test262 diff, focused suites) is running. It lands together with #944/#945/#946 as separate commits.

10 days ago

Implemented and pushed to main in 6f5d692b.

Each assembled CreateDynamicFunction program must now contain exactly one function expression whose captured source slice starts and ends at the constructor-owned boundary. The check runs before evaluation in all four constructors. It compares slice pointer and length, so it is O(1), allocation-free, and does not rescan large bodies.

Evidence:

  • Parent: all 16 constructor/body combinations were accepted and the side-effect probe produced 1.
  • Candidate: all 16 throw the constructor realm's SyntaxError, no injected code runs, and the matrix produces 160.
  • Eight controls remain valid: trailing // and leading --> bodies in Function, GeneratorFunction, AsyncFunction, and AsyncGeneratorFunction.
  • Exact-parent Debug test262: 1,659 affected cases, 0 fail before and 0 fail after (Function 509, GeneratorFunction 23, AsyncFunction 18, AsyncGeneratorFunction 23, Annex B 1,086).
  • Parser module: 227/227. Focused four-constructor matrix: pass.

Sign in to comment on this issue.