fix: add stack depth accounting to rawEquals - #892
Open
prasanna8585 wants to merge 1 commit into
Open
Conversation
rawEquals (which implements jsonnet's == and != operators) recursed
natively via the Go call stack when comparing nested arrays or objects,
with no stack-depth accounting anywhere in the call chain.
A self-referential jsonnet value compared with == (e.g. {a: $} == {a: $})
causes rawEquals to recurse indefinitely, consuming memory until OOM-kill
or hitting the Go runtime's stack limit.
This is the same bug class already fixed in manifestJSON/manifestYaml/
manifestToml builtins (depth-counter pattern seeded from i.stack.limit),
but rawEquals was not covered by that sweep.
Fix: add a depth int parameter to rawEquals, seeded from i.stack.limit
at each of its 5 entry points, decremented on each of the 2 recursive
call sites (array element comparison, object field comparison), erroring
with 'max equality depth exceeded' when exhausted -- consistent with the
existing manifest-builtin error message pattern.
Author
|
Hi @johnbartholomew @rohitjangid, Just checking in to see if you've had a chance to look at the patch strategy for this. I'm happy to spin up the code changes for the fix in a private fork if that helps speed things up. Thanks again for your time! |
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.
Problem
rawEqualsinbuiltins.go, which implements jsonnet's==and!=operators, recurses natively via the Go call stack when comparing nested arrays or objects. It has no stack-depth accounting anywhere in the call chain.A self-referential jsonnet value compared with
==:causes
rawEqualsto recurse into itself indefinitely, consuming memory until the process is killed by the OS (OOM) or the Go runtime's stack limit is hit.Precedent
This is the same bug class already fixed in
builtinManifestJSONEx,builtinManifestYamlDoc, andbuiltinManifestTomlEx(depth-counter pattern seeded fromi.stack.limit, decremented per recursive call, erroring with "max manifest depth exceeded").rawEqualswas never covered by that sweep and remains vulnerable to the identical trigger pattern.Fix
Adds a
depth intparameter torawEquals, seeded fromi.stack.limit(the interpreter's own configured stack limit, default 500) at all 5 entry points, and decremented on each of the 2 recursive call sites (array element comparison, object field comparison). Errors with"max equality depth exceeded, possible infinite recursion"when exhausted — matching the existing manifest-builtin error message style exactly.Testing
Verified with
{a: $} == {a: $}— with the fix, this now returns a clean error instead of hanging indefinitely with unbounded memory growth.