Skip to content

Reduce panic-driven internal control flow#6

Merged
AlexandreYang merged 2 commits into
mainfrom
dd/reduce-panic-internal-control-flow
Mar 9, 2026
Merged

Reduce panic-driven internal control flow#6
AlexandreYang merged 2 commits into
mainfrom
dd/reduce-panic-internal-control-flow

Conversation

@AlexandreYang

Copy link
Copy Markdown
Member

Summary

Converts internal panics in hot paths to explicit fatal errors, reserving panics for truly unrecoverable conditions (public API misuse, user handler crashes).

Changes

  • interp/vars.go: Added internalErrorf helper that records assertion failures via exit.fatal instead of panicking. Converted panics in lookupVar, setVarWithIndex, and assignVal to use it.
  • interp/api.go: Converted Reset() panic for zero-value Runner to exit.fatal + early return. Added explicit fatal-error check in Run() after Reset() so the error surfaces without relying on the recover wrapper.
  • interp/handler.go: Kept HandlerCtx panic (public API, genuine programmer error) but clarified the doc comment.
  • interp/allowed_paths_internal_test.go: Added TestRunZeroValueRunnerReturnsError to verify the new explicit error path.

The recover() in Run() is retained as a safety net for panics from user-provided handlers (validated by existing TestRunRecoversPanic).

Testing

  • All existing tests pass (go test ./...)
  • New test verifies zero-value Runner returns an explicit error instead of panicking

PR by Bits
View session in Datadog

Comment @DataDog to request changes

@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Mar 9, 2026

Copy link
Copy Markdown

View session in Datadog

Bits Dev status: ✅ Done

CI Auto-fix: Disabled | Enable

Comment @DataDog to request changes

@AlexandreYang
AlexandreYang merged commit f2dee7a into main Mar 9, 2026
7 checks passed
@AlexandreYang
AlexandreYang deleted the dd/reduce-panic-internal-control-flow branch March 9, 2026 09:47
julesmcrt added a commit that referenced this pull request May 12, 2026
…cs; close coverage gaps

Four review items from @AlexandreYang on PR #237 addressed in one
commit:

1. Promote the -P-above-sandbox passthrough invariant from a local
   comment into the package docstring so callers can reason about
   when cd -P diverges from bash on container-style sandboxes.

2. Improve the diagnostic emitted when ChangeDir is nil (the
   RunCommand-child case used by find -exec / -execdir / xargs).
   Was "cd: not supported in this runner" — generic and opaque to
   users. Now reads "cd: cannot change directory from inside
   find -exec/-execdir or xargs (child invocations are isolated)"
   so the user knows the cause is intentional isolation.

3. Add a scenario for HOME pointing outside the sandbox
   (errors/home_outside_sandbox.yaml). Existing scenarios only
   covered HOME unset and HOME empty; the rejection path where
   resolveOperand succeeds but changeDir rejects was untested.

4. Add four hardening unit tests for the HostPrefix re-prefix
   logic in resolveSymlinks: unrelated absolute target gets
   re-prefixed, already-prefixed target is left alone, target
   exactly equal to hp is left alone, and the
   /mnt/host vs /mnt/hostile prefix-collision boundary is
   correctly distinguished by the hp+sep check.

P3 review items (#5 Windows ACL test, #6 isCd literal-name
defer-skip) are intentionally not addressed — both are flagged
as not blocking and are forward-looking notes; replies posted
on the threads.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants