Bound user-defined function call depth instead of overflowing the stack - #46
Open
arcusbuilds wants to merge 1 commit into
Open
Bound user-defined function call depth instead of overflowing the stack#46arcusbuilds wants to merge 1 commit into
arcusbuilds wants to merge 1 commit into
Conversation
A recursive user-defined function (def f: f; f) recursed without limit until the goroutine stack overflowed. Go treats that as a fatal error rather than a panic, so recover() cannot catch it and the calling process dies -- with no mitigation available to the caller, since a compiled *Program cannot be inspected for recursion. execContext.depth was already incremented for every user-defined call but never read back. bindCallContexts is the single choke point for both value- and path-mode calls, so one comparison there covers every call shape. The limit reuses the existing maxRecurseDepth value (10000), roughly 13 MB of stack at ~1.3 KB per level. The depth error is an ordinary error value, so try catches it, matching the existing errRecurseDepthLimit precedent. The README and CHANGELOG say the limit bounds call depth and not total work, and that untrusted queries still need a timeout. Fixes DataDog#30 Signed-off-by: Srijan Keshri <srijankeshri007@gmail.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.
Fixes #30.
execContext.depthwas already incremented for every user-defined function call inbindCallContextsbut never read back, so this adds the missing comparison.bindCallContextsis the single choke point —execCall(value mode) andcollectAssignPaths(path mode) both route through it — so one check covers everycall shape.
Per your answers on the issue: the limit reuses
maxRecurseDepth(10,000), there is nonew sentinel error (deferred to #37), and the README wording plus fuzz seeds are included
here for you to call.
Verified terminating, each returning a normal error:
def f: f; fdef f: .|f; fdef f(x): f(x); f(.)def f($x): f($x); f(.)def f: {a:f}; fdef f: .a = f; fdef f: path(f); f[recurse(.)]from the issue list needs nothing — it already returnserrRecurseCycle.The limit bounds call depth, not total work — please read this part.
The depth error is catchable by
try, matching the existingerrRecurseDepthLimitprecedent. That means a recursive function whose error handler also recurses absorbs the
limit and evaluates its body twice per level — E(d) = 2·E(d+1) — so it runs unboundedly
instead of returning:
def f: try f catch f; ffatal error: stack overflow, rc=2def f: f? // f; ffatal error: stack overflow, rc=2def f: (f?, f?); ffatal error: stack overflow, rc=2I confirmed the doubling is exponential rather than merely slow by scaling
maxCallDepthlocally: limits of 12/14/16/18 gave 2 ms / 9 ms / 33 ms / 144 ms — ×2 per level.
So this is not a regression — master dies on all three, and this branch fixes every
shape the issue actually lists. But the failure mode changes from an instant crash to an
unkillable hang, which for the in-process remote-config case in the issue may be the worse
of the two. I've worded the README and CHANGELOG to say the limit bounds call depth and
not total work, and to tell callers to bound untrusted queries with a timeout, rather than
implying the guard alone makes them safe.
The real cure is making the error uncatchable — a sentinel registered in
isControlFlowError, sotrycannot absorb it, matching jq where the recursion error isuncatchable. That is exactly the error-shape work you deferred to #37, and I did not want
to pre-empt that call. But your "this can wait until #37" was made before either of us knew
catchability creates an exponential hang, so it seemed worth putting back in front of you.
Happy to add it here if you'd rather not ship the gap — it's a small diff on top of this one.
One more, lower stakes:
def f: [f]; fterminates but takes ~2.4 s and builds a230 KB error string, because
execArrayConstructwraps with"in array construction: "at each of the 10,000 levels — O(n²) string copying. That wrapping predates this change;
the guard only makes it reachable instead of fatal. Same "bounds depth, not work" theme.
It is also why that query is not in the fuzz seeds.
Benchmarks: no movement.
Small_Def680 B/op 16 allocs/op,Small_Bind432 B/op8 allocs/op,
Complex_TolerantMap2936 B/op 124 allocs/op — identical to master when runback to back on the same machine; the guard is one integer comparison and allocates
nothing. I did not regenerate
docs/BENCHMARKS.mdsince no number changed — happy to ifyou'd prefer it run regardless.
Tests: rebased onto current
master(through #44).go test . -count=1andgo vet ./...pass../jqtestalready fails onmasterfor reasons unrelated to thischange, and both the count and the exact set drift run to run — 18–19 failures observed,
with the two
reduce range(10001)deep-nesting cases atjq.testlines 2602/2616appearing intermittently on master and on this branch alike. Across repeated runs of each,
every failure on this branch also occurs on
master; there are no new ones.