Skip to content

NOTRACE_DISPATCH breaks tracing #96636

Closed
Closed
@brandtbucher

Description

@brandtbucher

Most of our quickened instructions start by asserting that tracing isn't enabled, and end by calling NOTRACE_DISPATCH(), which skips tracing checks. This is safe for many of our specializations (like those for BINARY_OP and COMPARE_OP, which operate on known safe types), but not all of them. The problem is that any time we decref an unknown object, arbitrary finalizers could run and enable tracing.

It seems that our current practice is predicated on the idea that it's okay for tracing to start "sometime in the future", since we make no guarantees about when finalizers run. There are two issues with this:

  • Trace functions may temporarily trace some frames, but not others.
  • As mentioned, the interpreter loop is littered with asserts that will fail if this happens.

Here's a sort-of-minimal reproducer of what this can look like on debug builds:

Python 3.11.0rc1+ (heads/3.11:bb0dab5c48, Sep  6 2022, 22:29:10) [GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class C:
...     def __init__(self, x):
...         self.x = x
...     def __del__(self):
...         if self.x:
...             breakpoint()
... 
>>> def f(x):
...     C(x).x, C
... 
>>> for _ in range(8):
...     f(False)
... 
>>> f(True)
--Return--
> <stdin>(6)__del__()->None
(Pdb) next
python: Python/ceval.c:3076: _PyEval_EvalFrameDefault: Assertion `cframe.use_tracing == 0' failed.
Aborted

In this case LOAD_ATTR_INSTANCE_VALUE's NOTRACE_DISPATCH() invalidates LOAD_GLOBAL_MODULE's assert(cframe.use_tracing == 0).

One option is to just rip out the asserts, which solves the more serious issue of crashing debug builds. However, I think that the best solution may be to stop using NOTRACE_DISPATCH() altogether. It's really hard to get right, and only saves us a single memory load and bitwise |. I benchmarked a branch that uses DISPATCH() for all instructions, and the result was "1.00x slower" than main. @markshannon tried the same thing, and got "1.01x slower". So going this route isn't free, but also not a huge penalty for correctness and peace-of-mind.

Flagging as a release blocker, since 3.11 is affected.

@markshannon @sweeneyde @pablogsal

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.11only security fixes3.12bugs and security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usagerelease-blockertype-bugAn unexpected behavior, bug, or errortype-crashA hard crash of the interpreter, possibly with a core dump

    Projects

    • Status

      Done

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions