bpo-39318: Catch a failure in NamedTemporaryFile to prevent leaking a descriptor #17985
Conversation
… descriptor This happened in the real world because someone mocked _TemporaryFileWrapper to raise an OSError. See https://nedbatchelder.com/blog/202001/bug_915_solved.html for more details.
| @@ -543,12 +543,18 @@ def NamedTemporaryFile(mode='w+b', buffering=-1, encoding=None, | |||
| file = _io.open(fd, mode, buffering=buffering, | |||
| newline=newline, encoding=encoding, errors=errors) | |||
|
|
|||
| return _TemporaryFileWrapper(file, name, delete) | |||
| except BaseException: | |||
There was a problem hiding this comment.
I would recommend to not use BaseException, but simply Exception here. Otherwise there is still a small race condition, e.g. if there is some KeyboardInterrupt after _io.open already created the file instance. Then you would run exactly in the same problem as before, that os.close is called twice.
There was a problem hiding this comment.
I don't understand the bad scenario you are describing. If there is a KeyboardInterrupt, then BaseException will catch it, and either the first or the second except clause will handle it (depending on exactly when the Ctrl-C happens), and the objects will be freed.
If we change to Exception, then KeyboardInterrupts won't be caught at all, and none of the clean up will happen.
There was a problem hiding this comment.
The bytecode will look sth like:
2 0 SETUP_EXCEPT 14 (to 16)
3 2 LOAD_GLOBAL 0 (_io)
4 LOAD_METHOD 1 (open)
6 LOAD_FAST 0 (fd)
... (other args ignored here)
8 CALL_METHOD 1
... assume KeyboardInterrupt is catched exactly here
10 STORE_FAST 1 (file)
12 POP_BLOCK
14 JUMP_FORWARD 30 (to 46)
4 >> 16 DUP_TOP
18 LOAD_GLOBAL 2 (BaseException)
20 COMPARE_OP 10 (exception match)
22 POP_JUMP_IF_FALSE 44
24 POP_TOP
26 POP_TOP
28 POP_TOP
...
It means that the file object already exists, and once the GC cleans it up, that file object will close the fd. However, now our exception handler runs, and also calls os.close(fd) again.
There was a problem hiding this comment.
Yikes, it seems like there's no way to prevent a leak in all cases with open(fd, closefd=True), because you can't atomically transfer ownership of the fd to the new file object.
I can't think of a good solution to this off the top of my head. However, leaking an fd in a rare KeyboardInterrupt situation is preferable to having a potential double-close; the latter causes a latent error that could explode unexpectedly, while the former is just an unlikely waste of resources.
There was a problem hiding this comment.
Well, I think if you just catch Exception, you are really safe here (I mean, ignore things like PyThreadState_SetAsyncExc...). If open returns successfully without a normal Exception, then the file object is created, and it will just immediately exit the except block, without possibly raising any other normal Exception.
There was a problem hiding this comment.
On https://bugs.python.org/issue39318, Serhiy says, "This issue is more complex. I am working on it."
| except BaseException: | ||
| _os.unlink(name) | ||
| _os.close(fd) | ||
| raise | ||
|
|
||
| try: | ||
| return _TemporaryFileWrapper(file, name, delete) | ||
| except BaseException: |
There was a problem hiding this comment.
Also here, better Exception instead of BaseException.
|
On https://bugs.python.org/issue39318, Serhiy says, "This issue is more complex. I am working on it." |
This happened in the real world because someone mocked
_TemporaryFileWrapper to raise an OSError. See
https://nedbatchelder.com/blog/202001/bug_915_solved.html for more
details.
https://bugs.python.org/issue39318