Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-39318: Catch a failure in NamedTemporaryFile to prevent leaking a descriptor #17985

Closed
wants to merge 2 commits into from
Closed

bpo-39318: Catch a failure in NamedTemporaryFile to prevent leaking a descriptor #17985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90e8da7
bpo-39318: Catch a failure in NamedTemporaryFile to prevent leaking a…
nedbat Jan 13, 2020
6e5c7fb
📜🤖 Added by blurb_it.
blurb-it[bot] Jan 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Lib/tempfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@albertz albertz Jan 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On https://bugs.python.org/issue39318, Serhiy says, "This issue is more complex. I am working on it."

_os.unlink(name)
_os.close(fd)
raise

try:
return _TemporaryFileWrapper(file, name, delete)
except BaseException:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, better Exception instead of BaseException.

_os.unlink(name)
file.close()
raise

if _os.name != 'posix' or _sys.platform == 'cygwin':
# On non-POSIX and Cygwin systems, assume that we cannot unlink a file
# while it is open.
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2020-01-13-13-36-05.bpo-39318.236VwO.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a file descriptor leak in NamedTemporaryFile under unusual circumstances.