Crash guard: read dumpability in order, and stop losing crashes to scope and naming - #8808
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
… payload inherits
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c94f3f1daf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Class bodies excluded: `class C: abort = ...` binds C.abort, not abort. | ||
| for node in _iter_executable(scope, enter_classes = False): |
There was a problem hiding this comment.
When the call is inside the same class body as the binding, excluding the entire class subtree loses Python's class-namespace lookup: from os import abort; class C: abort = lambda: None; abort() invokes the lambda, but the detector retains the imported alias and reports an unsuppressed crash. The added fixture only covers a module-level call after the class, so class-local calls remain a false CI failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Also real, also declining. Getting this right means teaching the analyser Python's class-namespace lookup, so a call lexically inside a class body sees that body's bindings while a call outside does not, which is a scoping model the guard does not otherwise have. The case is from os import abort plus a class that rebinds abort and calls it from its own body. The fixture I added covers the direction that actually loses a crash, a module-level call after the class, and that is the one that fails open. This one fails closed, so its worst outcome is a visible CI failure on a file nobody has written.
ceab6e7 to
adacf54
Compare
for more information, see https://pre-commit.ci
A payload inherits the dumpability its parent set, but the state was read from direct prctl calls only. A script that cleared dumping, called a local helper that put it back, and then exec'd a crash was therefore still credited with the clear, and a real core dump went unreported. _dumpable_writes now optionally counts a bare call to a local helper as whatever that helper leaves dumpability at, under the same rules the direct path uses: bare names only, and an async def that is never awaited runs none of its body.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… clear still lands
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
…c is the builtin Six path-sensitivity gaps, each pinned by a fixture that fails before the change: - a finally body always runs, so a restore there is not a conditional write - a function body runs after the module, so a global assigned below the def binds - a definite rebind also rules out the branch values it replaced - a platform-guarded clear counts inside a helper the same as it does inline - dumpability carries through every nested exec, not just the first - builtins.exec is the builtin, spelled out Also caps the command-vector walk, which hit RecursionError on a deeply nested literal. Out of the scan's scope today, but it errored the whole check rather than skipping one file.
|
All six confirmed and fixed in The first four were the same root cause showing up in different places: certainty was being attributed to the wrong node.
While rescanning the whole repo I also hit a 58 tests pass. Both real crash sites are still detected with no violations reported. On convergence: the shipped behaviour has been stable for four rounds now, and every finding since round 2 has been a synthetic construction rather than a shape that occurs in the tree. I am treating this round as the last refinement pass. The remaining open disagreement is the class-local namespace lookup, which I still think needs structural scope modelling out of proportion to a shape that appears nowhere in the repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6d35a5720
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for owner_scope in (tree, scope) if scope is not tree else (tree,): | ||
| # A nested scope runs after the module body, so a global assigned below the | ||
| # `def` is still bound by the time the call gets there. | ||
| limit = _AFTER_EVERYTHING if owner_scope is tree and scope is not tree else position |
There was a problem hiding this comment.
When a function is invoked before a later module-level rebind, scanning the entire module gives the payload the value it has only after the call. For example, INNER = 'import os; os.abort()'; def run(): exec(INNER); run(); INNER = 'pass' aborts while INNER still contains the crashing script, but this limit resolves it to pass, so the guard reports no crash or violation and permits the core dump.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining, and worth stating plainly since it pairs with the shadowing item I fixed. You are right that a function invoked before a later module rebind sees the earlier value. But that item and this one ask for opposite resolutions of the same name: one wants the local binding, one wants the value at the call site. Both are correct about Python, and satisfying both means resolving each name at the position of the call that reaches it, which is a call graph. That is out of proportion for a test guard whose input is this repo's test files.
There was a problem hiding this comment.
When two nested scopes define the same helper name, this flat function table can attribute a call to an unrelated definition. For example, if outer defines and calls a no-op configure() before exec('import os; os.abort()'), while a later unrelated function defines another configure() that clears dumpability, the latter wins in _functions_by_name; the exec payload is then incorrectly marked as inheriting suppression and a real core dump is allowed through.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining. Resolving helper calls lexically means giving the function table a real scope chain, which is the same structural change the class-local namespace item asks for. Neither shape occurs in the tree: no test file defines the same helper name in two nested scopes, and the failure needs the unrelated later definition to be the one that clears dumpability. Note 887c410 does handle the common half of this, a local rebinding of the helper name.
for more information, see https://pre-commit.ci
Five more path and scope gaps, each pinned by a fixture that fails before: - a name a function assigns anywhere is local throughout it, so a global of that name never reaches the body, even above the assignment - only the first operand of a short circuit certainly runs - a default argument or decorator runs where the def sits, not when it is called - a lambda is a scope, so its parameters shadow an imported crash alias - a string signal name is a TypeError and delivers nothing, so it is not a crash The first is a regression from the previous commit: binding globals late for nested scopes was right, but it has to respect local shadowing.
…erable Both follow from the previous commit: - requiring a libc receiver rejected an aliased handle, so lib = ctypes.CDLL(None) then lib.prctl(4, 0, ...) read as no suppression at all. Aliases bound from a CDLL call now count, while a mock named prctl still does not. - the outermost comprehension clause is evaluated where it sits, so its iterable is certain even though the body may never run.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
tools._libc.prctl(...) is the convention in test_bypass_permissions.py, and the receiver check I added rejected it because the attribute name did not match. The libc names are now compared with underscores and case stripped.
1 participant
Summary
Six ways the deliberate-crash guard still reads a file wrong, found by driving the detector directly rather than by a failing run. Four let a real core dump through, two fail CI on code that is already safe.
Each one has a fixture, and all six fail on current
mainand pass here.sigquit_is_a_core_dumping_signalrebinding_inside_an_unrelated_functiondumpability_restored_before_the_crashexec_payload_reached_by_nameclass_body_runs_with_its_enclosing_scopesuppression_above_a_nested_execThe six
SIGQUIT is a core-dumping signal.
_FATAL_SIGNAL_NUMBERSomitted 3, sosignal.raise_signal(3)was invisible. Its default action on Linux is terminate and dump, same as the rest of the table.A class body runs with its enclosing scope.
_iter_executableskippedClassDefalongside the function bodies, but a class body executes the moment the class is defined.class Probe: prctl(4, 0, ...); ctypes.string_at(0)was reported despite being suppressed. OnlyFunctionDef,AsyncFunctionDefandLambdaneed a separate call.Rebinding is the scope's own business.
_rebound_nameswalked the whole subtree, so a nesteddef unrelated(): abort = mockdisarmed a module-levelfrom os import abort; abort()and lost a real SIGABRT. It now walks the scope's own executable path plus its parameters.Dumpability is a state, not a flag.
_clears_dumpable_beforeaccepted any earlierprctl(4, 0, ...)without checking whether a later call restored it, soprctl(4, 0); prctl(4, 1); ctypes.string_at(0)passed. The guard's owndumpable_set_back_to_onefixture documents that this should be reported. The setting nearest before the crash now decides.An exec payload is usually one name away. The nested-script recursion only accepted a literal, so
INNER = "import os; os.abort()"; exec(INNER)was never analysed. It folds through the snippet's own environment now.Suppression carries into an exec. The recursion analysed the inner string as a fresh dumpable process, so
prctl(4, 0, ...); exec("import os; os.abort()")was reported even though the prctl covers it._nested_scriptsyields whether dumpability was already cleared, and the inner violation is dropped when it was.Not changed
_sequence_envstays flat by name. Its docstring makes the trade deliberately, that a false name collision only adds a candidate string to read while a miss loses the script a child runs, and that is the right way round for a guard.Verification
The whole-repo scan still finds both real deliberate-crash sites,
test_torch_device_probe.pyandtest_rag_embeddings.py, and reports no violations for either, so nothing about the shipped suppression changed.Follow-up to #8788.