with statements are not ensuring that __exit__ is called if __enter__ succeeds

@rhettinger

BPO	29988
Nosy	@rhettinger, @gpshead, @ncoghlan, @nirs, @njsmith, @markshannon, @embray, @serhiy-storchaka, @jdemeyer, @1st1, @ryanhiebert, @xgid, @jack1142
PRs	Protect C-implemented context managers, like lock, from ctrl-C. Fixes bpo-29988. #1799 bpo-29988: Only check evalbreaker after calls and on backwards egdes. #18334
Dependencies	bpo-31344: f_trace_opcodes frame attribute to switch to per-opcode tracing

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-12-10.17:37:50.631>
created_at = <Date 2017-04-04.23:42:57.982>
labels = ['interpreter-core', 'type-bug', '3.9', '3.10']
title = 'with statements are not ensuring that __exit__ is called if __enter__ succeeds'
updated_at = <Date 2021-12-10.17:37:50.630>
user = 'https://github.com/njsmith'

bugs.python.org fields:

activity = <Date 2021-12-10.17:37:50.630>
actor = 'Mark.Shannon'
assignee = 'none'
closed = True
closed_date = <Date 2021-12-10.17:37:50.631>
closer = 'Mark.Shannon'
components = ['Interpreter Core']
creation = <Date 2017-04-04.23:42:57.982>
creator = 'njs'
dependencies = ['31344']
files = []
hgrepos = []
issue_num = 29988
keywords = ['patch']
message_count = 60.0
messages = ['291148', '291157', '294272', '294296', '294400', '294402', '294404', '294434', '297177', '297179', '297180', '297182', '297185', '297186', '297189', '297190', '301235', '301248', '301274', '301277', '301278', '301285', '301289', '301360', '301429', '301553', '301557', '301566', '301573', '301601', '301609', '301610', '301611', '301620', '301630', '301660', '301667', '301668', '301672', '301675', '301682', '301698', '301717', '301721', '301771', '301869', '312874', '321225', '321318', '350123', '350134', '352026', '352039', '352199', '352212', '352457', '353415', '372499', '386711', '389922']
nosy_count = 15.0
nosy_names = ['rhettinger', 'gregory.p.smith', 'ncoghlan', 'nirs', 'njs', 'Mark.Shannon', 'erik.bray', 'serhiy.storchaka', 'jdemeyer', 'yselivanov', 'deleted0524', 'ryanhiebert', 'xgdomingo', 'jack1142', 'Ami Fischman']
pr_nums = ['1799', '18334']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29988'
versions = ['Python 3.9', 'Python 3.10']

You might hope the interpreter would enforce the invariant that for 'with' and 'async with' blocks, either '__(a)enter__' and '__(a)exit__' are both called, or else neither of them is called. But it turns out that this is not true once KeyboardInterrupt gets involved – even if we write our (async) context manager in C, or otherwise guarantee that the actual '__(a)enter__' and '__(a)exit__' methods are immune to KeyboardInterrupt.

The invariant is *almost* preserved for 'with' blocks: there's one instruction (SETUP_WITH) that atomically (wrt signals) calls '__enter__' and then enters the implicit 'try' block, and there's one instruction (WITH_CLEANUP_START) that atomically enters the implicit 'finally' block and then calls '__exit__'. But there's a gap between exiting the 'try' block and WITH_CLEANUP_START where a signal can arrive and cause us to exit without running the 'finally' block at all. In this disassembly, the POP_BLOCK at offset 7 is the end of the 'try' block; if a KeyboardInterrupt is raised between POP_BLOCK and WITH_CLEANUP_START, then it will propagate out without '__exit__' being run:

In [2]: def f():
...: with a:
...: pass
...:

In [3]: dis.dis(f)
2 0 LOAD_GLOBAL 0 (a)
3 SETUP_WITH 5 (to 11)
6 POP_TOP

3 7 POP_BLOCK
8 LOAD_CONST 0 (None)
>> 11 WITH_CLEANUP_START
12 WITH_CLEANUP_FINISH
13 END_FINALLY
14 LOAD_CONST 0 (None)
17 RETURN_VALUE

For async context managers, the race condition is substantially worse, because the 'await' dance is inlined into the bytecode:

In [4]: async def f():
...: async with a:
...: pass
...:

In [5]: dis.dis(f)
2 0 LOAD_GLOBAL 0 (a)
3 BEFORE_ASYNC_WITH
4 GET_AWAITABLE
5 LOAD_CONST 0 (None)
8 YIELD_FROM
9 SETUP_ASYNC_WITH 5 (to 17)
12 POP_TOP

3 13 POP_BLOCK
14 LOAD_CONST 0 (None)
>> 17 WITH_CLEANUP_START
18 GET_AWAITABLE
19 LOAD_CONST 0 (None)
22 YIELD_FROM
23 WITH_CLEANUP_FINISH
24 END_FINALLY
25 LOAD_CONST 0 (None)
28 RETURN_VALUE

Really the sequence from 3 BEFORE_ASYNC_WITH to 9 SETUP_ASYNC_WITH should be atomic wrt signal delivery, and from 13 POP_BLOCK to 22 YIELD_FROM likewise.

This probably isn't the highest priority bug in practice, but I feel like it'd be nice if this kind of basic language invariant could be 100% guaranteed, not just 99% guaranteed :-). And the 'async with' race condition is plausible to hit in practice, because if I have an '__aenter__' that's otherwise protected from KeyboardInterrupt, then it can run for some time, and any control-C during that time will get noticed just before the WITH_CLEANUP_START, so e.g. 'async with lock: ...' might complete while still holding the lock.

The traditional solution would be to define single "super-instructions" that do all of the work we want to be atomic. This would be pretty tricky here though, because WITH_CLEANUP_START is a jump target (so naively we'd need to jump into the "middle" of a hypothetical new super-instruction), and because the implementation of YIELD_FROM kind of assumes that it's a standalone instruction exposed directly in the bytecode. Probably there is some solution to these issues but some cleverness would be required.

A alternative approach would be to keep the current bytecode, but somehow mark certain stretches of bytecode as bad places to run signal handlers. The eval loop's "check for signal handlers" code is run rarely, so we could afford to do relatively expensive things like check a lookaside table that says "no signal handlers when 13 < f_lasti <= 22". Or we could steal a bit in the opcode encoding or something.

My first thought would be to inject a wordcode instruction that's essentially an eval loop directive: "ATOMIC_UNTIL <offset>"

ATOMIC_UNTIL would have at least the following properties:

checks for signals are skipped until the given offset is reached
checks to release the GIL are skipped until the given offset is reached

It may also have the following defensive coding property:

only wordcode instructions on a pre-approved whitelist are permitted during atomic execution blocks (i.e. we'd only add them to the whitelist on an as needed basis, to minimise the risk of undesirable side effects, like being able to use wordcode manipulation to put an entire loop inside an atomic block)

The last bit would likely have undesirable performance implications, so it would probably be reasonable to only enable the check for debug builds, rather than always enforcing it.

This is either a "won't fix" or an "impossible to fix" depending on your point of view.

PATIENT:
It hurts whenever I press ctrl-C
DOCTOR:
Then don't press ctrl-C

The problem is that ctrl-C can provoke an interrupt at any point in the program, and thus break presumed invariants.
try-finally has the same problem.

From the observable side effects it is indistinguishable whether an interrupt occurs after the last instruction in an __enter__ function or after the first (side-effect-less) instruction after the __enter__.
Likewise the last pre-exit and first in-exit instructions are effectively the same from the point of view from observable side-effects, but are different from the point of view of the handling of exceptions.

Right, fixing this bug alone can't make programs control-C safe in general. But there exist techniques to make __enter__/exit methods safe WRT control-C, and if we fix this bug *and* apply those techniques then you can get some meaningful guarantees.

For example, threading.Lock's __enter__ and __exit__ methods are written in C, so they are guaranteed to happen atomically WRT to control-C. Right now, it's possible for a 'with lock: ...' to raise KeyboardInterrupt with the lock still held, which is surprising and not what we would like. If this bug were fixed, then it would be guaranteed that the lock was always released.

(And I originally discovered this while implementing control-C handling in trio, which is pure-Python but also has a way to protect __(a)enter__/(a)exit methods against control-C. ...though that won't be 100% reliable until bpo-12857 or similar is fixed. See https://vorpus.org/blog/control-c-handling-in-python-and-trio/ for exhaustive details.)

If all you need is that

with foo:
   pass

guarantees that either both or neither of __enter__ and __exit__ are called, for C context managers, and only C context managers, then the fix is trivial.

To protect Python code would need a custom context manager wrapper

with ProtectsAgainstInterrupt(user_ctx_mngr()):
    do_stuff()

ProtectsAgainstInterrupt would need to be implemented in C and install a custom signal handler.

Nathaniel,

Do you have any way to reliably test for this failure mode?

If all you need is that with foo: pass guarantees that either both or neither of __enter__ and __exit__ are called, for C context managers, and only C context managers, then the fix is trivial.

It would be nice to have it for 'async with foo: pass' as well, which is a little less trivial because the 'await' dance is inlined into the bytecode (see the initial post in this bug), but basically yes.

Do you have any way to reliably test for this failure mode?

Unfortunately no, I haven't implemented one. Let's see, though...

The test I wrote for bpo-30039 demonstrates one way to trigger a signal on a precise bytecode, by writing some code in C that calls raise(SIGNALNUMBER), and then calling it immediately before the bytecode where we want the signal to be raised (simulating the situation where the signal happens to arrive while the C function is running -- note that raise() is convenient because unlike kill() it works cross-platform, even on Windows).

This might be sufficient for testing the 'async with' version; it looks like an __await__ method or iterator implemented in C and calling raise() would deliver a signal at a point that should be protected but isn't.

The tricky case is plain 'with'. We can write something like:

with lock:
    raise_signal()

and this gives bytecode like:

1 0 LOAD_NAME 0 (lock)
2 SETUP_WITH 12 (to 16)
4 POP_TOP

2 6 LOAD_NAME 1 (raise_signal)
8 CALL_FUNCTION 0
10 POP_TOP
12 POP_BLOCK
14 LOAD_CONST 0 (None)
>> 16 WITH_CLEANUP_START

So the problem is that at offset 8 is where we can run arbitrary code, but the race condition is if a signal arrives between offsets 12 and 16.

One possibility would be to set up a chain of Py_AddPendingCall handlers, something like:

int chain1(void* _) {
    Py_AddPendingCall(chain2, 0);
    return 0;
}
int chain2(void* _) {
    Py_AddPendingCall(chain3, 0);
    return 0;
}
int chain3(void* _) {
    raise(SIGINT);
    return 0;
}

(or to reduce brittleness, maybe use the void* to hold an int controlling the length of the chain, which would make it easy to run tests with chains of length 1, 2, 3, ...)

......except consulting ceval.c I see that currently this won't work, because it looks like if you call Py_AddPendingCall from inside a pending call callback, then Py_MakePendingCalls will execute the newly added callback immediately after the first one returns. It could be made to work by having Py_MakePendingCalls do a first pass to check the current length of the queue, and then use that as the bound on how many calls it makes.

On further thought, I think the way I'd write a test for this is:

(1) add a testing primitive that waits for N instructions and then injects a SIGINT. Probably this would require tweaking the definition of Py_MakePendingCalls like I described in my previous comment, and then some code like:

int sigint_timebomb(void* count_ptr) {
    intptr_t count = (intptr_t) count_ptr;
    if (count == 0)
        raise(SIGINT);
    else
        Py_AddPendingCall(sigint_timebomb, (void*) (count - 1));
    return 0;
}

(2) write a test like:

reached_crashpad = False
lock = threading.Lock()
i = 0
while not reached_crashpad:
    i += 1
    try:
        sigint_timebomb(i)
        with lock:
            1 + 1
        # this loop keeps executing instructions until any still-armed
        # timebombs go off
        while True:
            reached_crashpad = True
    except KeyboardInterrupt:
        # key invariant: signal can't leave the lock held
        assert not lock.locked()
    else:
        assert False  # can't happen

The idea here is that this sets off SIGINTs at every possible place throughout the execution of the 'with lock', while checking that the lock is always released, and without needing to hard code any information at all about what bytecode the compiler generated.

Nice analysis. I always assumed that with was safe from such race conditions, but it isn't.

One testing possibility worth considering: perhaps it would be make sense to have "start_try" and "start_finally" trace events, such that a trace function could be installed that injected signals at the worst possible time in the opcode evaluation?

I haven't fully thought through the possible implications of that approach, but it seems like it would be less fragile than counting code offsets.

Sorry, I misread Nathaniel's suggestion - the exhaustive search of all possible offsets already avoids the need to hardcode any code generation dependent details in the tests. So the trace function approach would just avoid the need for that search.

@nathaniel

Or we could steal a bit in the opcode encoding or something.

That seems like a very reasonable and easy-to-implement solution. It would generalize this check:

cpython/Python/ceval.c

Lines 1067 to 1071 in e82cf86

    
                       if (_Py_OPCODE(*next_instr) == SETUP_FINALLY) { 
        
                           /* Make the last opcode before 
        
                              a try: finally: block uninterruptible. */ 
        
                           goto fast_next_opcode; 
        
                       }

(@nathaniel Smith: nice blog post!)

> Or we could steal a bit in the opcode encoding or something.

That seems like a very reasonable and easy-to-implement solution. It > would generalize this check:

cpython/Python/ceval.c

Lines 1067 to 1071 in e82cf86

if (_Py_OPCODE(*next_instr) == SETUP_FINALLY) {

/* Make the last opcode before

a try: finally: block uninterruptible. */

goto fast_next_opcode;

}

Interesting; I didn't notice that bit before. It seems like that does at least try to guarantee that a signal can't interrupt between:

lock.acquire()
try:
    ...

which previously I assumed we couldn't make any guarantees about. But CPython at least does, or tries to. It would be good to generalize that.

The signal handler in this case is CPython's internal signal handling system thus any such onus falls on us...

The great signal handling hack of "set a flag that the interpreter loop checks on occasion" trick lasted a long time, but our VM has since evolved to be much more complicated than every bytecode being a safe interrupt point.

FYI - Most of the in-room conversation we tried to capture on this issue thread was at the core dev sprint a couple years ago (Sept 2017). It'll similarly take me a while to swap this context back in.

There is also the draft test case at ncoghlan#2

That needs updating to account for the eval loop changes since I last worked on it, though. (I think that conflict may also be keeping the bots from running, even after I updated the issue title to use the modern convention)

Is there a chance this will get resolved for 3.8? This is an important guarantee.

While there is technically still time, it'd take people spending dedicated time on it before the 3.8 release. I won't be.

Important? Sure. But not urgent; this bug is not new. We've lived with it for many releases already. I believe it has been around since 2.5 when with statements were introduced.

I expect to fix this is as part of the general improvements I am making to the bytecode, interpreter and compiler.
So it should be fixed for 3.9 but not for 3.8

It's also not unique to with statements - it applies to all finally clauses. The longstanding workaround when deterministic cleanup is absolutely critical has been to run the "real" application in a subthread, and devote the main thread to gracefully terminating the subthread when requested.

When cleanup is critical, but doing it in a deterministic order is less so, __del__ methods are often used to fill the gap (although they too can be interrupted by a subsequent Ctrl-C).

I also realized that allowing infinite loops in cleanup code to ignore Ctrl-C may actually be a tolerable outcome: in the worst case, users can still escalate to Ctrl-Break/kill -9/Force stop/etc and pull the entire OS process out from under the interpreter. It's not good, but may be worth it in order to better handle users pressing Ctrl-C multiple times.

As a note on the general pattern, a user at work diagnosed a ^C problem in their code when running on 2.7 to be due to Queue.get's

acquire()
try:
  ...
finally:
  release()

Pattern, with the KeyboardInterrupt triggering after acquire() but before the try is entered. so release() is never called.

A try finally pattern that probably alleviates this by entering the try block first might look like:

try:
acquire()
...
finally:
try:
release()
except ThreadError:
pass # interrupted before acquire() succeeded.

It'd be a shame if any with statements lock acquisition context managers need to be turned into that, but I _believe_ it'd be a viable workaround for the time being if this race is found to be biting anyone.

Belatedly clearing the issue assignment here - while I do still sometimes ponder this problem, I haven't been actively working on it since the 2017 core sprint where Greg & I made our last serious attempt at trying to improve the situation.

Mark's PR at #18334 looks very promising to me, though - my main request was just to bring over the tests I wrote at the 2017 core dev sprints, and confirm that the revised eval breaker logic solves the issue.

Does #1799 solve this issue
for synchronous with?

    with closing(this), closing(that):

If it does, can we backport this fix to python 3.6?

3.6 is used as system python for RHEL/Centos 8, will be used for at
least 5 years or so.

The bytecode instruction set has changed a lot since 3.6, so I think a backport would be impractical.

3.6 is in security fix only mode, so you'd need to take this up with Red Hat.

Navigation Menu

Repository navigation

with statements are not ensuring that exit is called if enter succeeds #74174

43 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	if (_Py_OPCODE(*next_instr) == SETUP_FINALLY) {
	/* Make the last opcode before
	a try: finally: block uninterruptible. */
	goto fast_next_opcode;
	}

with statements are not ensuring that __exit__ is called if __enter__ succeeds #74174

Description

Activity

njsmith commented on Apr 4, 2017

ncoghlan commented on Apr 5, 2017

deleted0524 commented on May 23, 2017

njsmith commented on May 23, 2017

markshannon commented on May 24, 2017

markshannon commented on May 24, 2017

njsmith commented on May 24, 2017

njsmith commented on May 25, 2017

jdemeyer commented on Jun 28, 2017

ncoghlan commented on Jun 28, 2017

ncoghlan commented on Jun 28, 2017

jdemeyer commented on Jun 28, 2017

embray commented on Jun 28, 2017

43 remaining items

gpshead commented on Aug 21, 2019

ncoghlan commented on Sep 11, 2019

rhettinger commented on Sep 12, 2019

gpshead commented on Sep 12, 2019

markshannon commented on Sep 12, 2019

ncoghlan commented on Sep 14, 2019

gpshead commented on Sep 27, 2019

ncoghlan commented on Jun 28, 2020

nirs commented on Feb 9, 2021

markshannon commented on Mar 31, 2021

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

with statements are not ensuring that exit is called if enter succeeds #74174