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

[njsmith]

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']

[njsmith]

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.

[ncoghlan]

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.

[deleted0524]

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.

[njsmith]

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.)

[markshannon]

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.

[markshannon]

Nathaniel,

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

[njsmith]

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.

[njsmith]

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.

[jdemeyer]

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

[ncoghlan]

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.

[ncoghlan]

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.

[jdemeyer]

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!)

[embray]

> 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.