Repository navigation
Bound parser source length and AST size - #3037
Conversation
a234b84 to
4a3af0a
Compare
4a3af0a to
dee5104
Compare
lahma
left a comment
There was a problem hiding this comment.
Reviewed as the incremental diff against sebros/improve-memory-accounting. This layer is fully opt-in (both limits default to null), the composition rule is the right one (per-call/prepared options can only tighten, Combine takes the minimum), and the coverage in ParsingLimitTests is excellent — the cache-poisoning cases (eval cache and dynamic-function cache keyed on constraints), prepared-code inheritance into nested eval, the async module thread hop carrying constraints, and CLR non-catchability are exactly the places this could have gone quietly wrong. Nice touches along the way: the source-offset padding length is now checked (and computed in long) before the padding string is allocated, the Function-constructor wrapper length is checked before the join materializes, DefaultModuleLoader stops reading the file at limit+1 chars instead of slurping it, and ClassDefinition no longer constructs a throwaway Engine just to parse the generated default constructor. Approving.
Small notes, none blocking:
ModuleimplementsIScriptOrModule.ParserOptions(ParserOptions?) with aninternal virtual ParserOptions ParserOptions => null;— the virtual should probably be declared nullable to match the interface rather than relying on however this dodges the nullability warning.DebugHandler.Evaluate(Prepared)now pushes a dedicated execution context carrying aScriptRecordwhere it previously executed in the paused frame without one. The environments/realm/strictness are copied from the active context so semantics should be unchanged, but it is a structural change to debugger evaluation beyond the limits feature — worth one sentence in the PR body.- The
_parsingConstraintsOverridesave/restore sites all restore infinally— good; anyone adding a new override site later should keep that discipline, since a leaked override silently tightens (or loosens) every subsequent parse on the engine.
Merge remains blocked below by the #3035 fix; this layer itself is good to go once the stack is re-rolled.
|
Follow-up to my approval: the The same run also has a single Test262 failure (an async |
dee5104 to
87546e4
Compare
87546e4 to
9a0b7e9
Compare
|
Rebased the rest of the stack onto current Where the stack was. The eleven layers above #3036 were 107 commits behind Every layer builds on all five TFMs, and the tip is green locally: Two decisions nobody approved, both because
A defect that is on Methods only, on purpose: A stale expectation, not a hole. Three net472 compile breaks and one flake. Unchanged from the August review: the five default flips — #3051 error redaction, #3054 I will keep merging bottom-up as each layer goes green and re-verify parentage after every merge. Shout if you want any of the above undone. |
Four security-stack layers (sebastienros#3037, sebastienros#3045, sebastienros#3046, sebastienros#3051) merged between this branch's base and its landing, and each added exactly the shape of setting the posture rule exists to classify. The parser bounds, the four module-graph limits and the ResultLimits bundle are restrictions and travel; detailed-error exposure and the require shim are grants and stay behind. Modules stops being excludable wholesale — the group now mixes its loader (a grant) with numeric limits (restrictions), so it joins the scanned set with each member classified. ResultLimits is class-typed and outside the reflective pin's value-typed scan, so it is classified in CopySecurityPosture's body instead, and the copy is by reference because the bundle is sealed and immutable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnghUC3EKrYa12xQ2LaYyd
Four security-stack layers (sebastienros#3037, sebastienros#3045, sebastienros#3046, sebastienros#3051) merged between this branch's base and its landing, and each added exactly the shape of setting the posture rule exists to classify. The parser bounds, the four module-graph limits and the ResultLimits bundle are restrictions and travel; detailed-error exposure and the require shim are grants and stay behind. Modules stops being excludable wholesale — the group now mixes its loader (a grant) with numeric limits (restrictions), so it joins the scanned set with each member classified. ResultLimits is class-typed and outside the reflective pin's value-typed scan, so it is classified in CopySecurityPosture's body instead, and the copy is by reference because the bundle is sealed and immutable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnghUC3EKrYa12xQ2LaYyd
#3219) * Workers: the host-facing types, the options group and the posture rule The foundation of the Worker feature (#3167), with no script surface at all: `typeof Worker` stays `undefined` on every engine, nothing is installed, and no engine runtime behaviour changes. What lands: - `Jint/WebApi/Workers/` — `WorkerProvider` (the host's answer to `new Worker(...)`, since Jint never starts a thread), `WorkerRequest` (what is being asked for, plus `CreateDefaultOptions()`), `WorkerConnection` (one live parent-worker pair) and the two enums. The connection is thread-safe now rather than later: `End()` is idempotent under *concurrent* callers, every property reads safely from any thread, and the end callback runs outside the lock so host code never runs under one. - `Options.WebApi.Workers` — `Provider` (null by default, which is what keeps the global uninstalled), `MaxWorkers` (16, a per-engine backstop and not the policy) and `MaxQueuedMessages` (16384, a backstop against a never-pumped worker rather than flow control), plus `WebApiFeatures.Workers = 1 << 24`, never in `Default`, and `UseWorkers` which sets flag and provider together. - `Options.CopySecurityPosture` — restrictions travel, grants never travel by implication. It copies the seven `Constraints` value settings, `Host.StringCompilationAllowed`, `AgentCanSuspend` and `Json.MaxParseDepth`, and lives beside the properties it names with three explicit classification lists: what is inherited, what is deliberately not (with the reason), and which option groups are excluded wholesale as grant-shaped. `CreateDefaultOptions()` returns a fresh `Options` each call: the termination token as a cancellation constraint, every parent constraint *factory* replayed (never an instance — those are single-engine-only), the copied posture, the feature mask minus network/storage/routing/nesting plus `Messaging` and `GlobalEvents`, and `DiagnosticsSink.Null`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Workers: pin the foundation, and the classification that outlives it Mechanism in Jint.Tests, third-party reachability in Jint.Tests.PublicInterface, each pin written so that removing the mechanism it names makes it fail. `OptionsSecurityPostureTests` is the one worth keeping past this feature: it reflects over the value-typed settings of `Options` and its `Constraints`, `Host` and `Json` groups and fails unless each is either copied by `CopySecurityPosture` or named as deliberately not inherited, and it does the same for the option groups themselves. It found one on its first run — `Options.Profiling`, which is now classified (host diagnostics, script cannot reach it, and its gate already defaults to refusing). `TheDefaultOptionsCopyTheParentsSecurityPosture` is a theory with one case per copied setting, so dropping a single line of the copy fails exactly the case that names it rather than one arbitrary assertion. `TypeofWorkerIsUndefinedEvenWithTheFlagAndProvider` pins that this change is not also the one that moves the engine: with the flag on, a provider registered and the web APIs enabled, a script still cannot tell any of this exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Workers: sync the design doc, and tell the truth about a pumped worker's budget `docs/design/web-workers.md` predates the finalized plan in #3167 and disagreed with it in nine places that matter. Rewritten decision for decision: - `close()` is not `terminate()`. The spec's *close a worker* discards the worker's own queued tasks and sets the closing flag; it pointedly does not abort the running script or empty the parent-side queue. The doc had them sharing one three-step teardown, which would kill `close(); flushMetrics();` and lose `postMessage(result); close()`. - A load failure fires a plain `Event`, not an `ErrorEvent`, and ends the connection as `StartupFailed` rather than `Terminated`. - The parent-side error relay is an internal per-connection hook gated on *notHandled*, sitting beside the `DiagnosticsSink` rather than being it — the sink is deliberately unsuppressible by script and cannot carry that gate. - Nesting is OFF by default; `MaxWorkers` (16) is a per-engine backstop that bounds a branching factor, not a tree, which is what the old text claimed. - Security posture: restrictions travel, grants never travel by implication, and the classification is pinned reflectively. - `importScripts` is present and throwing, because the spec's own step 1 for a module worker prescribes the throw. - The agent-cluster claim is corrected: HTML puts a dedicated worker in its creator's cluster, so refusing `SharedArrayBuffer` is Jint's isolation policy and not a fact about agents. - "Close both ports is already the shipped rule" is withdrawn: the shipped rule is the opposite one-sided close, and the worker rule is argued on its own merits (cost, not delivery). - The generation fence is per **port**, captured at port construction, which is what lets a transferred side rejoin the receiving engine's current cycle. - The `AWorkerCannotBeSentAMessagePort` pin is deleted; it contradicted #3197. Line-number citations are replaced with file-and-member ones throughout. Most of the old numbers were already wrong, and a wrong line number reads like a fact. #3215's transferable-streams paragraph is preserved, integrated into the transfer section beside the port transfer it rides on. The same correction lands in the XML docs, where it is load-bearing rather than merely explanatory. `WorkerRequest.CreateDefaultOptions` claimed a replayed `LimitMemory` never fires on a pumped worker. Since #3036 that is false: every event-loop job runs inside an allocation segment and is checked as it completes, carrying the operation state captured at registration across continuations and thread hops, so a replayed factory genuinely bounds each job chain. The worker budget is a pair — `OperationDeadlineConstraint` for time, `MemoryLimitConstraint` for allocations. The same change cites #3036's other consequence: instance copying is now loudly wrong, because `MemoryLimitConstraint.Attach` throws when one instance meets a second engine. `Options`'s wholesale exclusion of the `WebApi` group also stops glossing: its value settings are per-feature caps living on the sub-groups, and each rides with the feature it bounds rather than being a restriction that stayed behind by accident. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Workers: make the End()-idempotency pin actually race The pin claimed to prove that `WorkerConnection.End()` is idempotent under *concurrent* callers rather than merely repeated ones — which is the entire reason `TryEnd` takes a lock instead of testing and setting a `volatile bool`. It did not prove it. Replacing the lock with a plain check-then-set left the test passing five times out of five: `Parallel.For` ramps its workers up, so by the time the second one starts the first has long since set the flag and there is no race left to lose. The pin named a mechanism it could not see. A `Barrier` releases every racer into the same instant instead, the threads are reused across 64 rounds so that many attempts cost a handful of threads rather than thousands, and each round gets its own connection and its own callback counter — so the failure message names the round rather than an aggregate. The unlocked version now loses within the first couple of rounds, verified five times out of five, and the assertion that fails is the callback count, which is exactly the mechanism removed. Nothing here is timed: there is no wall-clock assertion, so a slow machine makes this test slower and never redder, and the joins carry a 60-second ceiling only so that an `End()` that deadlocked fails the test instead of hanging the run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Workers: classify the security settings the stack landed under us Four security-stack layers (#3037, #3045, #3046, #3051) merged between this branch's base and its landing, and each added exactly the shape of setting the posture rule exists to classify. The parser bounds, the four module-graph limits and the ResultLimits bundle are restrictions and travel; detailed-error exposure and the require shim are grants and stay behind. Modules stops being excludable wholesale — the group now mixes its loader (a grant) with numeric limits (restrictions), so it joins the scanned set with each member classified. ResultLimits is class-typed and outside the reflective pin's value-typed scan, so it is classified in CopySecurityPosture's body instead, and the copy is by reference because the bundle is sealed and immutable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnghUC3EKrYa12xQ2LaYyd * Workers: inherit the untrusted-code profile's expansion, never its marker #3060 landed the hardened profile while this branch was in review, and it resolved a question the design had left open in the profile's favour — almost. An untrusted parent's Engine.Options is the profile's expanded clone, so the posture copy already takes every value the expansion set and the factory replay already carries its budget constraints; propagating the marker itself would be strictly worse, because a marked options object re-expands at engine construction and that expansion clears the constraint registrations, including the cancellation constraint a worker's terminate() depends on. The unmarked worker loses only the diagnostics label, and the test pins both halves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnghUC3EKrYa12xQ2LaYyd --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…lly has Main has moved 181 commits past v4.16.0 and nothing records what an embedder has to react to. Most of it is invisible to a compiler: every entry below still compiles exactly as it did in 4.16 and behaves differently at run time, and six of them flip a default. `docs/v5-migration.md` is the artefact those entries go in. It is deliberately not a second README: a table per change, before/after code where it helps, and the rationale left in the pull request it cites. Seeded from git history, every claim checked against the tree rather than against the pull request title: * sebastienros#3054 `Interop.AllowWrite` true -> false. A projected CLR write is now silently ignored in sloppy mode and a TypeError in strict mode. * sebastienros#3056 `Interop.ArrayConversion` LiveView -> Copy. `Array.isArray` flips to true, `push`/`length` stop throwing, and CLR-side mutations after the crossing stop being visible. * sebastienros#3057 `Constraints.StackOverflowGuard` false -> true. RangeError instead of a process that ends with no exception in the log. * sebastienros#3058 `AgentCanSuspend` true -> false. `Atomics.wait` is a TypeError before a waiter is registered; `waitAsync` is untouched. * sebastienros#3052 namespace type discovery loses its implicit fallback to `Assembly.GetCallingAssembly()` / `GetExecutingAssembly()` / `Type.GetType(name)` and becomes the `AllowedAssemblies` allow-list. * sebastienros#3051 host exception, module-load and CLR-resolution messages are redacted from script; `ExposeDetailedErrors()` restores all three. * sebastienros#3035 concurrent `Engine` use fails fast, and an engine stays reserved for the lifetime of a returned async Task. * sebastienros#3036 `LimitMemory` charges allocations across async continuations, so a budget can now trip where one synchronous segment never reached it. * sebastienros#3252 every `*Async` entry reports the operation's failures through the task and only a usage error out of the call. * sebastienros#3248 an array-like `length` above 2^32-1 stops answering differently per target framework. * sebastienros#3037, sebastienros#3045, sebastienros#3046 add parser, module-graph and result bounds that all default to unlimited, so they change nothing until configured; sebastienros#3059 and sebastienros#3060 add the diagnostics and the hardened profile on top. Two entries the prompt for this work had slightly differently, both checked and written as the tree has them: the stack-overflow guard raises a catchable `RangeError`, not a `RecursionDepthOverflowException`, and `ArrayOperations` is internal, so sebastienros#3248 removes no public member — its break is what a script sees. Sections 2, 3 and 6 (removed API, renamed API, AOT) are explicit empty placeholders. Nothing public has been removed or renamed since v4.16.0, and a parallel task is measuring the AOT state. The target-framework section is written and marked pending: `Jint.csproj` still lists net462, and the net472 change is not on main yet. `Jint/AGENTS.md` gains the rule that makes the guide stay current — a change to anything in its public-contract table is a row in the guide, in the same pull request, including a change that breaks nothing at compile time. The root `AGENTS.md` is untouched; it is at 23 KB against a 24 KiB budget. README's "Branches and releases" described `main` alone and did not mention the `3.x` branch at all. It now has a row per live branch, in the same wording sebastienros#3291 gives the 4.x branch's own copy, plus a pointer to the guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S
…lly has (#3294) Main has moved 181 commits past v4.16.0 and nothing records what an embedder has to react to. Most of it is invisible to a compiler: every entry below still compiles exactly as it did in 4.16 and behaves differently at run time, and six of them flip a default. `docs/v5-migration.md` is the artefact those entries go in. It is deliberately not a second README: a table per change, before/after code where it helps, and the rationale left in the pull request it cites. Seeded from git history, every claim checked against the tree rather than against the pull request title: * #3054 `Interop.AllowWrite` true -> false. A projected CLR write is now silently ignored in sloppy mode and a TypeError in strict mode. * #3056 `Interop.ArrayConversion` LiveView -> Copy. `Array.isArray` flips to true, `push`/`length` stop throwing, and CLR-side mutations after the crossing stop being visible. * #3057 `Constraints.StackOverflowGuard` false -> true. RangeError instead of a process that ends with no exception in the log. * #3058 `AgentCanSuspend` true -> false. `Atomics.wait` is a TypeError before a waiter is registered; `waitAsync` is untouched. * #3052 namespace type discovery loses its implicit fallback to `Assembly.GetCallingAssembly()` / `GetExecutingAssembly()` / `Type.GetType(name)` and becomes the `AllowedAssemblies` allow-list. * #3051 host exception, module-load and CLR-resolution messages are redacted from script; `ExposeDetailedErrors()` restores all three. * #3035 concurrent `Engine` use fails fast, and an engine stays reserved for the lifetime of a returned async Task. * #3036 `LimitMemory` charges allocations across async continuations, so a budget can now trip where one synchronous segment never reached it. * #3252 every `*Async` entry reports the operation's failures through the task and only a usage error out of the call. * #3248 an array-like `length` above 2^32-1 stops answering differently per target framework. * #3037, #3045, #3046 add parser, module-graph and result bounds that all default to unlimited, so they change nothing until configured; #3059 and #3060 add the diagnostics and the hardened profile on top. Two entries the prompt for this work had slightly differently, both checked and written as the tree has them: the stack-overflow guard raises a catchable `RangeError`, not a `RecursionDepthOverflowException`, and `ArrayOperations` is internal, so #3248 removes no public member — its break is what a script sees. Sections 2, 3 and 6 (removed API, renamed API, AOT) are explicit empty placeholders. Nothing public has been removed or renamed since v4.16.0, and a parallel task is measuring the AOT state. The target-framework section is written and marked pending: `Jint.csproj` still lists net462, and the net472 change is not on main yet. `Jint/AGENTS.md` gains the rule that makes the guide stay current — a change to anything in its public-contract table is a row in the guide, in the same pull request, including a change that breaks nothing at compile time. The root `AGENTS.md` is untouched; it is at 23 KB against a 24 KiB budget. README's "Branches and releases" described `main` alone and did not mention the `3.x` branch at all. It now has a row per live branch, in the same wording #3291 gives the 4.x branch's own copy, plus a pointer to the guide. Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
ParsingLimitExceptionsemantics with engine ownership and memory accounting, including cleanup of pending loads, waiters, async ownership, and memory operation stateStack
This PR is stacked on #3036 -> #3035 -> #3030 and targets
sebros/improve-memory-accounting.Integration decisions
Validation
dotnet build Jint/Jint.csproj -c Release(all target frameworks)Jint.Tests.PublicInterface: 1,585 passed, 9 configuration skipsgit diff --checkThe full core suite completed 5,760 tests with one unrelated environment-dependent Date failure (year 10000 rendered as Dec 31 9999 in Pacific time). The full solution build could not restore
Test262Harness.Console1.1.2 because the required package proxy does not currently carry it; no alternate feed was used.