Guard native recursion and forwarding paths - #3877
Merged
Merged
Conversation
5 tasks
lahma
pushed a commit
to lahma/jint
that referenced
this pull request
Sep 9, 2026
Backport of PR sebastienros#3877 (commit 044f44a) from main. `StackGuard`'s backstop sat on `ScriptFunction`'s four entry points, so it covered recursion that goes back through interpreted code and nothing else. A recursion that stays inside native frames -- `Array.prototype.flat`/`join`/`toLocaleString` walking a nested array, `JSON.stringify` walking a nested graph, a chain of `bind`s or `Proxy` wrappers forwarding a single call, a host `ClrFunction` or delegate that calls back into the engine -- never reached it, and ended the process with a native stack overflow that no constraint and no `catch` can see. `EnsureNativeStackHeadroom` is the same probe placed on those paths. It is gated on `Options.Constraints.StackOverflowGuard` alone (`_graphGuardEnabled` renamed `_nativeBackstopEnabled` for what it now covers), because these paths hold ref structs, pooled arrays and identity-bearing state and so cannot take `MaxExecutionStackCount`'s stack-hopping lane. `BindFunction` also returns its pooled argument buffer in a `finally`, so an exceptional exit no longer leaks it. Adapted for 4.x: - `Jint/Native/HostFunction.cs` does not exist on this branch -- it is a v5 public API -- so that hunk is dropped. The host-callable surfaces 4.x does have, `ClrFunction` and `DelegateWrapper`, both take the probe as on main. - `JsonSerializer` has no `EnterContainer` here: main's result-limit accounting (`_limits`, `_depth`, `CountProperties`) is part of the security stack 4.x does not carry. The probe goes to the three `_stack.Enter(value)` sites instead, which is where `EnterContainer` is called from on main. - One context conflict in `Engine.cs`: `SignalError` is still `SignalError` here (sebastienros#3845 is a separate backport), so `ConstructNativeFunction` is added above it unchanged. - Tests transcribed from NUnit to xUnit, which is what 4.x's test projects still are, and every engine in them asks for `StackOverflowGuard` explicitly: the guard is opt-in on this branch where it defaults on upstream, so a default 4.x engine is deliberately still unprotected. The `HostFunction` half of the host-callable test goes with the type; the `ClrFunction` and `Constructor` halves stay. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLCujwvKtTvtWD9f6RTyiF
lahma
added a commit
that referenced
this pull request
Sep 9, 2026
…4007) * Backport: Guard native recursion and forwarding paths Backport of PR #3877 (commit 044f44a) from main. `StackGuard`'s backstop sat on `ScriptFunction`'s four entry points, so it covered recursion that goes back through interpreted code and nothing else. A recursion that stays inside native frames -- `Array.prototype.flat`/`join`/`toLocaleString` walking a nested array, `JSON.stringify` walking a nested graph, a chain of `bind`s or `Proxy` wrappers forwarding a single call, a host `ClrFunction` or delegate that calls back into the engine -- never reached it, and ended the process with a native stack overflow that no constraint and no `catch` can see. `EnsureNativeStackHeadroom` is the same probe placed on those paths. It is gated on `Options.Constraints.StackOverflowGuard` alone (`_graphGuardEnabled` renamed `_nativeBackstopEnabled` for what it now covers), because these paths hold ref structs, pooled arrays and identity-bearing state and so cannot take `MaxExecutionStackCount`'s stack-hopping lane. `BindFunction` also returns its pooled argument buffer in a `finally`, so an exceptional exit no longer leaks it. Adapted for 4.x: - `Jint/Native/HostFunction.cs` does not exist on this branch -- it is a v5 public API -- so that hunk is dropped. The host-callable surfaces 4.x does have, `ClrFunction` and `DelegateWrapper`, both take the probe as on main. - `JsonSerializer` has no `EnterContainer` here: main's result-limit accounting (`_limits`, `_depth`, `CountProperties`) is part of the security stack 4.x does not carry. The probe goes to the three `_stack.Enter(value)` sites instead, which is where `EnterContainer` is called from on main. - One context conflict in `Engine.cs`: `SignalError` is still `SignalError` here (#3845 is a separate backport), so `ConstructNativeFunction` is added above it unchanged. - Tests transcribed from NUnit to xUnit, which is what 4.x's test projects still are, and every engine in them asks for `StackOverflowGuard` explicitly: the guard is opt-in on this branch where it defaults on upstream, so a default 4.x engine is deliberately still unprotected. The `HostFunction` half of the host-callable test goes with the type; the `ClrFunction` and `Constructor` halves stay. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLCujwvKtTvtWD9f6RTyiF * Backport test: forwarding chains deep enough to overflow a 1 MiB stack on every frame size On Linux 4.x's BindFunction and Proxy hops are light enough that ten thousand of them fit in the test thread's 1 MiB stack, so the chain returned and the guard had nothing to catch; the CI leg reported "none" where a RangeError was expected. Fifty thousand layers exhaust that stack for any plausible frame size, so the probe fires before the process would die, on every platform. The traversal routes are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qq1kWgfU9v38r7yDcjxK48 * Backport test: the forwarding chains run on a stack they overflow at any frame size On Linux x64 ten thousand of this branch's bound-call or proxy hops fit in the test thread's 1 MiB stack, so the chain returned and the CI leg saw "none"; a five-fold deeper chain then died on ARM64 before the probe could answer. A 256 KiB thread runs out at a depth the probe sees first on every platform, with main's original depth. The traversal routes are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qq1kWgfU9v38r7yDcjxK48 * Backport test: a tail-called forwarding hop may complete, on any runtime On Linux x64 the bound-call chain answered "none" even on a 256 KiB thread: the System V JIT turns this branch's bound-call forward into a tail call, so the hop consumes no stack and the chain cannot overflow, exactly as the .NET Framework JIT already did for the proxy forward. The guard promises a catchable error whenever the stack does run out, not that it must, so both forwarding routes accept completion; the traversal and construct routes stay strict. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qq1kWgfU9v38r7yDcjxK48 --------- Co-authored-by: Sébastien Ros <sebastienros@gmail.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
lahma
added a commit
to lahma/jint
that referenced
this pull request
Sep 14, 2026
…verge on purpose Two documentation fixes for the gap the previous commit exposed. Jint/Constraints/AGENTS.md gains a gotcha beside the MaxRecursionDepth one that already hands off to StackOverflowGuard. It states where the probes actually are — the dispatcher one in Engine.CallNativeFunction, the leaf ones at the top of ClrFunction/HostFunction/DelegateWrapper/BindFunction's Call — why neither kind is removable, that with default options the probe is the only thing between a deep script and a process-killing native overflow rather than a catchable error, and that Function._probesOwnNativeStack is a claim about IL kept honest by StackOverflowGuardTests.EveryFunctionWhoseCallProbesDeclaresThatItDoes. It goes here rather than in Jint/AGENTS.md's gotchas because this is the file that owns StackOverflowGuard and the recursion bounds, and because the reader who is about to edit a dispatcher is warned in the code itself. Which is the second half: the "ensure logic is in sync between Call, Construct, engine.Invoke and JintCallExpression!" comment has been stale since sebastienros#3877 added the dispatcher probe to Engine's copies only. All four copies now say what they do still have to keep in sync — the call-stack frame and its pop in a finally, the recursion-depth check, the ScriptFunction-vs-native branch — and the two that have no dispatcher probe say that this is deliberate and must not be "fixed", since the four interop and forwarding types already probe at the top of their own Call and a probe there would be the second one on every host method call. Jint/Constraints/AGENTS.md: 28,024 -> 30,698 bytes against the 32,768 budget. AgentInstructionFileTests is green, as is the rest of Jint.Tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQwq9NqYqG3kk9M8CKdfdJ
lahma
added a commit
to lahma/jint
that referenced
this pull request
Sep 14, 2026
… a measured NO-GO An Ultra capture of a DOM-property-read loop on sebastienros#4013 put ObjectInstance.UnwrapFromGetter's subtree at 29.1% of the page-loop thread, and 17.0% of that subtree in StackGuard.ProbeStackHeadroom, because a host accessor read probes the native stack twice: JintMemberExpression.ReadAfterOwnMissUncached ObjectInstance.UnwrapJsValue ObjectInstance.UnwrapFromGetter (ObjectInstance.cs:1173) Engine.Call(Function, ...) (Engine.cs:5144) Engine.CallNativeFunction (Engine.cs:5180) <- probe ClrFunction.Call (ClrFunction.cs:65) <- probe verified from a live stack trace. A method call does not: it goes through JintCallExpression's own native branch, which has no dispatcher probe. Closing the redundancy — a flag on Function saying its own Call probes, read by a callee-aware StackGuard overload so the dispatcher could skip, preserving the guard on every route — MEASURED AS NO CHANGE and is not part of this commit. Six rounds, paired, alternating order, idle box, DefaultJob: the subject row -1.35% [-3.65, +2.07] with sign agreement 1 round of 6, every BrowserPropertyReadBenchmark DOM row no change. Removing one of the two probes is below what a six-round paired benchmark on this hardware can resolve. There is no executable change under Jint/ here; every route probes exactly as before. What does land is everything the investigation produced that stands without it. Jint/Constraints/AGENTS.md gains two gotchas beside the MaxRecursionDepth one that already hands off to StackOverflowGuard: where the probes are and why neither the dispatcher's nor the four leaf ones can be deleted, that with the default options the probe is the only thing between a deep script and a process-killing native overflow rather than a catchable error, and that the double probe on the accessor route is known, deliberate and already measured — so the next person to spot it looks it up instead of re-deriving it. Jint.Benchmark/AGENTS.md carries the table, because that is the file whose subject is numbers, and because of what the table shows besides the verdict: DataPropertyRead, a plain data-property read that cannot reach the changed method, read +1.19% [+0.65, +3.48] 6/6 with an interval excluding zero. That is the second calibration artefact this campaign has caught on a row that provably cannot execute the change, and it was only visible because the class carried a floor row — which is now stated as a rule for new classes. The "ensure logic is in sync between Call, Construct, engine.Invoke and JintCallExpression!" comment has been stale since sebastienros#3877 added the dispatcher probe to Engine's copies only. All four copies now say what they do still have to keep in sync, and the two without a dispatcher probe say the asymmetry is deliberate and must not be "fixed" — a probe there would be the second one on every host method call. StackOverflowGuardTests.ExactlyTheInteropAndForwardingFunctionsProbeTheNativeStack reads the IL of every Function in the engine assembly and asserts that exactly ClrFunction, HostFunction, DelegateWrapper and BindFunction probe in their Call, with a non-empty assertion first so the equality cannot hold vacuously. Deleting ClrFunction.cs:65 aborts the test run with a native stack overflow inside HostNativeRecursionGuardTests; it now fails an assertion naming ClrFunction first, confirmed by doing it. Jint.Benchmark/HostAccessorReadBenchmark is the row the repository did not have: AccessorRead the subject, MethodCall a control that dispatches through JintCallExpression and so cannot reach the dispatcher probe, DataPropertyRead the floor. Jint.Tests, Jint.Tests.PublicInterface and AgentInstructionFileTests are green on net8.0, net10.0 and net472. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQwq9NqYqG3kk9M8CKdfdJ
lahma
added a commit
to lahma/jint
that referenced
this pull request
Sep 14, 2026
… a measured NO-GO An Ultra capture of a DOM-property-read loop on sebastienros#4013 put ObjectInstance.UnwrapFromGetter's subtree at 29.1% of the page-loop thread, and 17.0% of that subtree in StackGuard.ProbeStackHeadroom, because a host accessor read probes the native stack twice: JintMemberExpression.ReadAfterOwnMissUncached ObjectInstance.UnwrapJsValue ObjectInstance.UnwrapFromGetter (ObjectInstance.cs:1173) Engine.Call(Function, ...) (Engine.cs:5144) Engine.CallNativeFunction (Engine.cs:5180) <- probe ClrFunction.Call (ClrFunction.cs:65) <- probe verified from a live stack trace. A method call does not: it goes through JintCallExpression's own native branch, which has no dispatcher probe. Closing the redundancy — a flag on Function saying its own Call probes, read by a callee-aware StackGuard overload so the dispatcher could skip, preserving the guard on every route — MEASURED AS NO CHANGE and is not part of this commit. Six rounds, paired, alternating order, idle box, DefaultJob: the subject row -1.35% [-3.65, +2.07] with sign agreement 1 round of 6, every BrowserPropertyReadBenchmark DOM row no change. Removing one of the two probes is below what a six-round paired benchmark on this hardware can resolve. There is no executable change under Jint/ here; every route probes exactly as before. What does land is everything the investigation produced that stands without it. Jint/Constraints/AGENTS.md gains two gotchas beside the MaxRecursionDepth one that already hands off to StackOverflowGuard: where the probes are and why neither the dispatcher's nor the four leaf ones can be deleted, that with the default options the probe is the only thing between a deep script and a process-killing native overflow rather than a catchable error, and that the double probe on the accessor route is known, deliberate and already measured — so the next person to spot it looks it up instead of re-deriving it. Jint.Benchmark/AGENTS.md carries the table, because that is the file whose subject is numbers, and because of what the table shows besides the verdict: DataPropertyRead, a plain data-property read that cannot reach the changed method, read +1.19% [+0.65, +3.48] 6/6 with an interval excluding zero. That is the second calibration artefact this campaign has caught on a row that provably cannot execute the change, and it was only visible because the class carried a floor row — which is now stated as a rule for new classes. The "ensure logic is in sync between Call, Construct, engine.Invoke and JintCallExpression!" comment has been stale since sebastienros#3877 added the dispatcher probe to Engine's copies only. All four copies now say what they do still have to keep in sync, and the two without a dispatcher probe say the asymmetry is deliberate and must not be "fixed" — a probe there would be the second one on every host method call. StackOverflowGuardTests.ExactlyTheInteropAndForwardingFunctionsProbeTheNativeStack reads the IL of every Function in the engine assembly and asserts that exactly ClrFunction, HostFunction, DelegateWrapper and BindFunction probe in their Call, with a non-empty assertion first so the equality cannot hold vacuously. Deleting ClrFunction.cs:65 aborts the test run with a native stack overflow inside HostNativeRecursionGuardTests; it now fails an assertion naming ClrFunction first, confirmed by doing it. Jint.Benchmark/HostAccessorReadBenchmark is the row the repository did not have: AccessorRead the subject, MethodCall a control that dispatches through JintCallExpression and so cannot reach the dispatcher probe, DataPropertyRead the floor. Jint.Tests, Jint.Tests.PublicInterface and AgentInstructionFileTests are green on net8.0, net10.0 and net472. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQwq9NqYqG3kk9M8CKdfdJ
lahma
added a commit
that referenced
this pull request
Sep 14, 2026
… a measured NO-GO (#4073) An Ultra capture of a DOM-property-read loop on #4013 put ObjectInstance.UnwrapFromGetter's subtree at 29.1% of the page-loop thread, and 17.0% of that subtree in StackGuard.ProbeStackHeadroom, because a host accessor read probes the native stack twice: JintMemberExpression.ReadAfterOwnMissUncached ObjectInstance.UnwrapJsValue ObjectInstance.UnwrapFromGetter (ObjectInstance.cs:1173) Engine.Call(Function, ...) (Engine.cs:5144) Engine.CallNativeFunction (Engine.cs:5180) <- probe ClrFunction.Call (ClrFunction.cs:65) <- probe verified from a live stack trace. A method call does not: it goes through JintCallExpression's own native branch, which has no dispatcher probe. Closing the redundancy — a flag on Function saying its own Call probes, read by a callee-aware StackGuard overload so the dispatcher could skip, preserving the guard on every route — MEASURED AS NO CHANGE and is not part of this commit. Six rounds, paired, alternating order, idle box, DefaultJob: the subject row -1.35% [-3.65, +2.07] with sign agreement 1 round of 6, every BrowserPropertyReadBenchmark DOM row no change. Removing one of the two probes is below what a six-round paired benchmark on this hardware can resolve. There is no executable change under Jint/ here; every route probes exactly as before. What does land is everything the investigation produced that stands without it. Jint/Constraints/AGENTS.md gains two gotchas beside the MaxRecursionDepth one that already hands off to StackOverflowGuard: where the probes are and why neither the dispatcher's nor the four leaf ones can be deleted, that with the default options the probe is the only thing between a deep script and a process-killing native overflow rather than a catchable error, and that the double probe on the accessor route is known, deliberate and already measured — so the next person to spot it looks it up instead of re-deriving it. Jint.Benchmark/AGENTS.md carries the table, because that is the file whose subject is numbers, and because of what the table shows besides the verdict: DataPropertyRead, a plain data-property read that cannot reach the changed method, read +1.19% [+0.65, +3.48] 6/6 with an interval excluding zero. That is the second calibration artefact this campaign has caught on a row that provably cannot execute the change, and it was only visible because the class carried a floor row — which is now stated as a rule for new classes. The "ensure logic is in sync between Call, Construct, engine.Invoke and JintCallExpression!" comment has been stale since #3877 added the dispatcher probe to Engine's copies only. All four copies now say what they do still have to keep in sync, and the two without a dispatcher probe say the asymmetry is deliberate and must not be "fixed" — a probe there would be the second one on every host method call. StackOverflowGuardTests.ExactlyTheInteropAndForwardingFunctionsProbeTheNativeStack reads the IL of every Function in the engine assembly and asserts that exactly ClrFunction, HostFunction, DelegateWrapper and BindFunction probe in their Call, with a non-empty assertion first so the equality cannot hold vacuously. Deleting ClrFunction.cs:65 aborts the test run with a native stack overflow inside HostNativeRecursionGuardTests; it now fails an assertion naming ClrFunction first, confirmed by doing it. Jint.Benchmark/HostAccessorReadBenchmark is the row the repository did not have: AccessorRead the subject, MethodCall a control that dispatches through JintCallExpression and so cannot reach the dispatcher probe, DataPropertyRead the floor. Jint.Tests, Jint.Tests.PublicInterface and AgentInstructionFileTests are green on net8.0, net10.0 and net472. Claude-Session: https://claude.ai/code/session_01WQwq9NqYqG3kk9M8CKdfdJ Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
8 tasks
lahma
added a commit
to lahma/jint
that referenced
this pull request
Sep 19, 2026
A prototype chain is built by script, so its depth is an input.
ObjectInstance's [[Get]], [[Set]] and [[HasProperty]] each resolved it by
calling the same method on Prototype, which is one native frame per link, so
let x = {}; for (let i = 0; i < 20000; i++) x = { __proto__: x }; x.missing
ended the process with a native stack overflow. Nothing was thrown and no catch
saw it, where the same depth through JSON.stringify or flat(Infinity) already
raised a catchable RangeError (sebastienros#3877). The same shape reached the write side
through `x.missing = 1`, the existence side through `'missing' in x`, and --
less obviously -- every identifier resolved inside `with (x) { ... }`, whose
object environment asks HasProperty.
All four walks are loops now: Get through GetFromPrototypeChain, the private
receiver-threading TryGetValue overload inline, Set through SetOnPrototypeChain
(from both fast-path arms and from OrdinarySetWithOwnDescriptor), and
HasProperty inline. An ordinary chain of any depth therefore resolves rather
than failing politely -- the frames are gone, not bounded.
A loop in the base class may only run the ordinary algorithm, and every
override of these three -- ArrayInstance, JsProxy, JsTypedArray, ObjectWrapper
and its Specialized forms, JsArguments, JsError, IteratorResult,
ModuleNamespace, ArrayLikeObject, NamedPropertyObject, NamespaceReference,
TypeReference, JsStorage -- calls base as its own tail, i.e. the base runs on
behalf of the derived object. A walk that probed GetOwnProperty itself would
silently skip a mid-chain override. So each walk hands the rest of the
algorithm to the first link it may not walk, exactly as
JintMemberExpression.ReadAfterOwnMissUncached already does. The read side keys
that on InternalTypes.ExoticGet | OwnValueHook, which already meant it. [[Set]]
and [[HasProperty]] have no such derived flag, so they ask for the positive
claim instead and walk only a link carrying InternalTypes.PlainObject, set
through the internal constructor by five types (JsObject, JsDate, GlobalObject,
NumberPrototype, and Prototype, the base of every built-in prototype), none of
which overrides one. That direction is the safe one: a link the walk declines
is resolved exactly as it always was.
No probe was added to the walk itself, for two reasons. Jint/Constraints/
AGENTS.md records the double native-stack probe on the accessor-read lane as a
measured NO-GO -- an Ultra capture put ProbeStackHeadroom at 17.0% of a 29.1%
subtree, and a six-round paired gate found no resolvable change -- and
ObjectInstance.Get is hotter still. And StackOverflowGuardTests
.ExactlyTheInteropAndForwardingFunctionsProbeTheNativeStack reads the probe
sites out of the IL and pins them to BindFunction, ClrFunction, DelegateWrapper
and HostFunction. The probe sits at the hand-over instead, which is off the
ordinary path and paid only by a chain that really does forward; that pin is
untouched and still passes, because it scans Function subclasses' Call and none
of the new sites is one.
The second instance of the same defect is JsProxy. A trapless proxy forwards
its whole algorithm to its target, so new Proxy(new Proxy(new Proxy(...))) is a
recursion script controls, and only [[Call]] and [[Construct]] probed. That
chain cannot be flattened -- each hop runs trap lookup and the invariant checks
on the way back out -- so it is probed once, in EnterProxyOperation (the renamed
AssertNotRevoked, which every proxy internal method already calls first). The
two hand-written probes in [[Call]] and [[Construct]] are gone because this is
the same point in the same order.
InternalTypes.PlainObject's comment now states the obligation the walks read it
as, and Jint.Tests/Runtime/PrototypeChainWalkTests asserts it over every object
reachable from a built engine rather than leaving it as prose: no type carrying
the flag overrides Get, Set or HasProperty. Its two behaviour tests prove the
other half, that a Proxy and an array mid-chain still run their own algorithms.
HostNativeRecursionGuardTests gains the depth tests, which assert the answer
rather than a RangeError for an ordinary chain and a catchable RangeError for a
proxy chain; on net472 the trapless proxy read takes the same carve-out the
trapless proxy call already has, the JIT turning the forwarding call into a tail
call. PrototypeChainReadBenchmark gains AbsentNameRead, the only row whose cost
is the walk to the end of the chain rather than a cached hit.
Closes sebastienros#4076
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 24, 2026
lahma
added a commit
that referenced
this pull request
Sep 25, 2026
…nk (#4078) * Walk the prototype chain in a loop instead of one native frame per link A prototype chain is built by script, so its depth is an input. ObjectInstance's [[Get]], [[Set]] and [[HasProperty]] each resolved it by calling the same method on Prototype, which is one native frame per link, so let x = {}; for (let i = 0; i < 20000; i++) x = { __proto__: x }; x.missing ended the process with a native stack overflow. Nothing was thrown and no catch saw it, where the same depth through JSON.stringify or flat(Infinity) already raised a catchable RangeError (#3877). The same shape reached the write side through `x.missing = 1`, the existence side through `'missing' in x`, and -- less obviously -- every identifier resolved inside `with (x) { ... }`, whose object environment asks HasProperty. All four walks are loops now: Get through GetFromPrototypeChain, the private receiver-threading TryGetValue overload inline, Set through SetOnPrototypeChain (from both fast-path arms and from OrdinarySetWithOwnDescriptor), and HasProperty inline. An ordinary chain of any depth therefore resolves rather than failing politely -- the frames are gone, not bounded. A loop in the base class may only run the ordinary algorithm, and every override of these three -- ArrayInstance, JsProxy, JsTypedArray, ObjectWrapper and its Specialized forms, JsArguments, JsError, IteratorResult, ModuleNamespace, ArrayLikeObject, NamedPropertyObject, NamespaceReference, TypeReference, JsStorage -- calls base as its own tail, i.e. the base runs on behalf of the derived object. A walk that probed GetOwnProperty itself would silently skip a mid-chain override. So each walk hands the rest of the algorithm to the first link it may not walk, exactly as JintMemberExpression.ReadAfterOwnMissUncached already does. The read side keys that on InternalTypes.ExoticGet | OwnValueHook, which already meant it. [[Set]] and [[HasProperty]] have no such derived flag, so they ask for the positive claim instead and walk only a link carrying InternalTypes.PlainObject, set through the internal constructor by five types (JsObject, JsDate, GlobalObject, NumberPrototype, and Prototype, the base of every built-in prototype), none of which overrides one. That direction is the safe one: a link the walk declines is resolved exactly as it always was. No probe was added to the walk itself, for two reasons. Jint/Constraints/ AGENTS.md records the double native-stack probe on the accessor-read lane as a measured NO-GO -- an Ultra capture put ProbeStackHeadroom at 17.0% of a 29.1% subtree, and a six-round paired gate found no resolvable change -- and ObjectInstance.Get is hotter still. And StackOverflowGuardTests .ExactlyTheInteropAndForwardingFunctionsProbeTheNativeStack reads the probe sites out of the IL and pins them to BindFunction, ClrFunction, DelegateWrapper and HostFunction. The probe sits at the hand-over instead, which is off the ordinary path and paid only by a chain that really does forward; that pin is untouched and still passes, because it scans Function subclasses' Call and none of the new sites is one. The second instance of the same defect is JsProxy. A trapless proxy forwards its whole algorithm to its target, so new Proxy(new Proxy(new Proxy(...))) is a recursion script controls, and only [[Call]] and [[Construct]] probed. That chain cannot be flattened -- each hop runs trap lookup and the invariant checks on the way back out -- so it is probed once, in EnterProxyOperation (the renamed AssertNotRevoked, which every proxy internal method already calls first). The two hand-written probes in [[Call]] and [[Construct]] are gone because this is the same point in the same order. InternalTypes.PlainObject's comment now states the obligation the walks read it as, and Jint.Tests/Runtime/PrototypeChainWalkTests asserts it over every object reachable from a built engine rather than leaving it as prose: no type carrying the flag overrides Get, Set or HasProperty. Its two behaviour tests prove the other half, that a Proxy and an array mid-chain still run their own algorithms. HostNativeRecursionGuardTests gains the depth tests, which assert the answer rather than a RangeError for an ordinary chain and a catchable RangeError for a proxy chain; on net472 the trapless proxy read takes the same carve-out the trapless proxy call already has, the JIT turning the forwarding call into a tail call. PrototypeChainReadBenchmark gains AbsentNameRead, the only row whose cost is the walk to the end of the chain rather than a cached hit. Closes #4076 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Probe the proxy forward, not the entry of every proxy operation Review of #4078. AssertNotRevoked had been renamed to EnterProxyOperation and given the probe, so all 17 internal methods probed at their entry -- but a *trapped* proxy reaches its trap through CallTrap -> ICallable.Call, where the callee probes for itself: ScriptFunction's four entry points carry the backstop, the interop and forwarding leaves carry theirs, and a callable proxy carries [[Call]]'s. The entry probe was therefore a redundant third probe on exactly the shape real code ships -- every reactivity library (Vue 3, MobX, immer) puts a get trap on every object it proxies, and Jint.Tests.Browser runs a Vue 3 TodoMVC. That is the redundancy Jint/Constraints/AGENTS.md already records as a measured NO-GO for the dispatcher probes, and ProbeStackHeadroom is a NoInlining wrapper around RuntimeHelpers.TryEnsureSufficientExecutionStack rather than something free. A trapped chain was never the bug: only the trapless forward, `return target.<op>(...)`, recursed unguarded. The probe moves there, into ForwardToTarget(target), which hands the target back so every site reads as the one thing it is -- `return ForwardToTarget(target).Get(property, receiver)` -- and a forward added later without a probe stands out beside its siblings. All 24 forward sites take it (13 internal methods, each with a trapless arm and a CLR-handler-declined arm, plus IsSpecArray and ToObject which have no trap at all). That includes the three forwarding through a property rather than a call: Prototype is GetPrototypeOf() and Extensible is IsExtensible(), both virtual and both overridden here, so both recurse. No proxy internal method has a trapless path that does not forward to the target. AssertNotRevoked keeps its original name and body -- the rename was only worth having while the probe lived there -- and [[Call]]/[[Construct]] keep their own entry probes, byte-identical to main, because they are the dispatcher for everything reached through them. Narrowing those two the same way is a pre-existing question this change deliberately leaves alone. Net effect: identical coverage of the shape that actually recursed, and no added cost on a trapped proxy. DeepProxyChains now runs read, write and `in` under both handlers. The trapped rows are the evidence that the entry probe was redundant: they pass against unmodified main -- base ObjectInstance.cs and JsProxy.cs restored and rebuilt, on all three target frameworks -- so the callee's own probe already turned that chain into a catchable RangeError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Walk a host's shaped prototype instead of declining it The paired gate on #4078 blocked it: on shaped prototypes (HostPrototypeShapeBenchmark, PrototypeKind=Shape) DeepHitOverChain was +8.70% and AbsentNameMissOverChain +5.52% at eight rounds, while the dictionary rows improved -9.94% and -15.42%. Both chain rows are `'name' in obj`, so the walk they exercise is [[HasProperty]]'s -- and [[HasProperty]]'s and [[Set]]'s gates ask for the POSITIVE claim InternalTypes.PlainObject, which five constructors set by hand. SharedShapeObject -- what every JsObjectShape.Instantiate returns, i.e. every shaped host prototype -- overrides only Initialize() and passes InternalTypes.Object alone, so the walk declined a link it was entitled to walk, handed the rest of the chain back to the recursive path AND paid the new flag test per level: the old recursion plus the new overhead. The read side never had this, because its gate (ExoticGet | OwnValueHook) is DERIVED per type and so self-maintaining. The claim is true for this type and the flag is now declared: as a LINK its [[Set]] and [[HasProperty]] are the ordinary algorithm the loop performs, the same virtual GetOwnProperty / ProbeOwnProperty / GetPrototypeOf the loop calls, and the same receiver. The interaction the flag's other readers have with BuiltinShapeMode is not a new one: ObjectInstance.Get, Set and CreateDataProperty read PlainObject as a STORAGE claim and each spells its test `== PlainObject` against `PlainObject | BuiltinShapeMode`, so a shaped object is excluded from all three for as long as its shape is installed, and once DeoptBuiltinShape has moved every slot into _properties it is the ordinary dictionary they assume. Object.prototype is that same pair today -- a Prototype (hence PlainObject) declared [JsObject(UseShape = true)] (hence BuiltinShapeMode) -- and every walk has resolved it that way all along. PrototypeChainWalkTests now asserts both directions over the same census, which is extended with a shaped chain built through the public JsObjectShape API since no engine builds one on its own. The converse -- nothing that overrides none of the named methods is unflagged -- is what would have caught this, and it names the offending type. Its allowlist holds the nineteen other in-box types the census reaches that are eligible and unflagged, with the reason: PlainObject is two claims in one, and taking it also switches on three storage fast lanes, which is a change to measure on its own terms. A shaped object is the exception, because those lanes exclude it by construction. The new regression is behavioural: a shaped chain answers `in`, hasOwnProperty, a shadowing write, an inherited setter's receiver and a get-only accessor's refusal. Jint.Tests.PublicInterface adds the direct statement that the chain is WALKED and not recursed -- twenty thousand shaped links, which answer when looped and raise the hand-over probe's RangeError when not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Make the forwarding-chain cases a hundred thousand hops deep Ten thousand bound-call hops came to fit the 1 MB test stack on linux x64 once main's frames got leaner, so the row completed instead of probing and the assertion that the probe fires failed there alone. A hundred thousand hops cannot fit on any runtime, which leaves the probe's RangeError, a tail call, or a dead host as the only outcomes - and the last is what the case exists to rule out. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Let a forwarding route complete when its frames fit the test stack Ten thousand bound-call hops came to fit the 1 MB test stack on linux x64 once main's frames got leaner, so that row completed instead of probing and its assertion that the probe fires failed there alone. Making the chains a hundred thousand deep instead shut down every Linux runner mid-run. What the case rules out is a dead host, and a route that fits has nothing to overflow, so the three rows now accept either the probe's RangeError or a clean completion - the contract the .NET Framework proxy route already had - and the depth goes back to ten thousand. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current
mainprotects interpreted function recursion, but recursive native algorithms and forwarding objects can still consume the CLR stack without enteringScriptFunction. DeepArray.prototype.flat/join/toLocaleString, unlimitedJSON.stringify, proxy/bound chains, and recursively re-entered host callables can therefore terminate the process instead of producing a catchable JavaScript error.Extend the existing default
StackOverflowGuardbackstop to those native boundaries. The change keeps the existing opt-out, leaves explicit result-depth limits in charge when configured, and returns pooled bound-call arguments infinallyblocks so a guarded unwind does not strand them. It also coversHostFunction,ClrFunction, delegate wrappers, proxy call/construct forwarding, and host-defined constructors reached throughEngine.The bound-call unwind exposes that the pooling regression test assumed the very next same-size rent was the JSON buffer. The corrected test verifies that buffer among the two valid same-size rents now returned by the guarded path.
On .NET Framework, the JIT can tail-call-optimize an otherwise empty proxy-call forwarding chain so it consumes no stack and legitimately completes. The public test accepts that optimized result for that one route on
net472; every path that actually recurses must still raise a catchableRangeError, and modern targets keep the stricter assertion for the proxy route too.This integrates and updates the safety intent of Nicolas Camarès' #2468 for the current stack-guard design, retaining attribution here while avoiding the stale branch's separate fixed-depth option and broad dispatch rewrite. The original PR remains open until this replacement passes CI and merges.
Validation:
Jint.slnxRelease build: 0 warnings/errorsHostNativeRecursionGuardTestsplus existingHostStackOverflowGuardTests: 22 passed on net8.0 and 22 on net10.0 after refreshing onto currentmainnet472cross-build withOS=Windows_NT: 0 warnings/errorsJsonPoolingInternalsTests,StackOverflowGuardTests, andEngineLimitTests: 27 passed on net8.0 and 27 on net10.0