NamedTemporaryFile could cause double-close on an fd if _TemporaryFileWrapper throws

[nneonneo]

BPO	39318
Nosy	@nedbat, @sethtroisi, @albertz, @serhiy-storchaka, @eryksun, @jstasiak, @sanjioh, @joernheissler
PRs	bpo-39318: Catch a failure in NamedTemporaryFile to prevent leaking a descriptor #17985

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = None
created_at = <Date 2020-01-13.04:56:47.144>
labels = ['3.8', 'type-bug', 'library', '3.9', '3.10']
title = 'NamedTemporaryFile could cause double-close on an fd if _TemporaryFileWrapper throws'
updated_at = <Date 2021-03-28.03:37:24.913>
user = 'https://bugs.python.org/nneonneo'

bugs.python.org fields:

activity = <Date 2021-03-28.03:37:24.913>
actor = 'eryksun'
assignee = 'serhiy.storchaka'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2020-01-13.04:56:47.144>
creator = 'nneonneo'
dependencies = []
files = []
hgrepos = []
issue_num = 39318
keywords = ['patch']
message_count = 10.0
messages = ['359888', '359892', '359935', '359957', '359959', '360040', '360041', '360071', '360084', '360104']
nosy_count = 12.0
nosy_names = ['nneonneo', 'nedbat', 'Seth.Troisi', 'Albert.Zeyer', 'serhiy.storchaka', 'paul_ollis', 'eryksun', 'jstasiak', 'sanjioh', 'joernheissler', 'hitbox', 'rekcartgubnohtyp']
pr_nums = ['17985']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue39318'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

[nneonneo]

tempfile.NamedTemporaryFile creates its wrapper like so:

try:
    file = _io.open(fd, mode, buffering=buffering,
                    newline=newline, encoding=encoding, errors=errors)

        return _TemporaryFileWrapper(file, name, delete)
    except BaseException:
        _os.unlink(name)
        _os.close(fd)
        raise

If _TemporaryFileWrapper throws any kind of exception (even KeyboardInterrupt), this closes fd but leaks a valid file pointing to that fd. The file will later attempt to close the fd when it is collected, which can lead to subtle bugs. (This particular issue contributed to this bug: https://nedbatchelder.com/blog/202001/bug_915_please_help.html)

This should probably be rewritten as:

try:
    file = _io.open(fd, mode, buffering=buffering,
                    newline=newline, encoding=encoding, errors=errors)
except:
    _os.unlink(name)
    _os.close(fd)
    raise

try:
    return _TemporaryFileWrapper(file, name, delete)
except BaseException:
    _os.unlink(name)
    file.close()
    raise

or perhaps use nested try blocks to avoid the _os.unlink duplication.

[albertz]

Instead of `except:` and `except BaseException:`, I think better use `except Exception:`.

For further discussion and reference, also see the discussion here: https://news.ycombinator.com/item?id=22028581

[serhiy-storchaka]

This issue is more complex. I am working on it.