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.

[serhiy-storchaka]

Problems:

1. In general, it is hard to avoid leaks because an exception like KeyboardInterrupt or MemoryError can be raised virtually anywhere, even before we save a file descriptor. We may rewrite the code so that it will use few simple bytecode instructions and disable interruption before these instructions. This may solve the problem in CPython, but alternate implementation will need to handle this theirself.

2. It is not safe to close a file descriptor if io.open() is failed, because it can already be closed in io.open(), and we do not know where it was closed or no. It can be solved by using the opener argument, but we will need to patch the name attribute of the file returned by io.open(), and it is not easy, because the type of the result of io.open() depends on the mode and buffering arguments, and only FileIO has writable name attribute. This will need additional complex code. We can also change the behavior of io.open(), make it either always closing fd or never closing it in case of error. But this will break working code, or introduce leaks in working code, so such change cannot be backported. In any case we should revisit all other cases of using io.open() with an open file descriptor in the stdlib (including TemporaryFile).

3. I afraid that removing a file while the file descriptor is open may not work on Windows. Seems this case is not well tested.

As for using except Exception instead of except BaseException, it is better to use the later, or even the bare except.

I worked on this issue yesterday, but then found new problems, it will take several days or weeks to solve them. As a workaround I suggest to skip temporary the test which monkeypatches _TemporaryFileWrapper.

[albertz]

Why is except BaseException better than except Exception here? With except Exception, you will never run into the problem of possibly closing the fd twice. This is the main important thing which we want to fix here. This is more important than missing maybe to close it at all, or unlink it.

[nneonneo]

Could this be solvable if closefd were a writable attribute? Then we could do

    file = None
    try:
        file = io.open(fd, ..., closefd=False)
        file.closefd = True
    except:
        if file and not file.closefd:
            os.close(fd)
        raise

I believe this should be bulletproof - a KeyboardInterrupt can happen anywhere in the try and we will not leak or double-close. Either file.closefd is set, in which case file owns the fd and will close it eventually, or file.closefd is not set in which case the fd needs to be manually closed, or file doesn't exist (exception thrown in io.open or while assigning file), in which case the fd still needs to be manually closed.

Of course, this can leak if a KeyboardInterrupt lands in the except - but that's not something we can meaningfully deal with. The important thing is that this shouldn't double-close if I analyzed it correctly.

This is a somewhat annoying pattern, though. I wonder if there's a nicer way to implement it so we don't end up with this kind of boilerplate everywhere.

[albertz]

If you anyway accept that KeyboardInterrupt can potentially leak, by just using except Exception, it would also be solved here.

[paulollis]

I think it is worth pointing out that the semantics of

f = ``open(fd, closefd=True)`` 

are broken (IMHO) because an exception can result in an unreferenced file
object that has taken over reponsibility for closing the fd, but it can
also fail without creating the file object.

Therefore it should be considered a bad idea to use open in this way. So
NamedTemporaryFile should take responsibility for closing the fd; i.e. it
should used closefd=False.

I would suggest that NamedTemporaryFile's code should be:

try:
    file = _io.open(fd, mode, buffering=buffering, closefd=False,
                    newline=newline, encoding=encoding, errors=errors)
    return _TemporaryFileWrapper(file, name, delete)
except BaseException:
    _os.close(fd)
    try:
        _os.unlink(name)
    except OSError:
        pass  # On windows the file will already be deleted.
    raise

And '_os.close(self.file.fileno())' should be added after each of the two calls
to 'self.file.close()' in _TemporaryFileCloser.

Some notes on the design choices here.

1. The exception handling performs the close *before* the unlink because:

   • That is the normal order that people expect.
   • It is most important that the file is closed. We cannot rule out the
     possibility of the unlink raising an exception, which could leak the fd.

2. BaseException is caught because we should not leak a file descriptor for
   KeyboardInterrupt or any other exception.

3. It is generally good practice to protect os.unlink with a try ... except
   OSError. So I think this is an appropriate way to prevent the Windows
   specific error.

   It will also suppress some other, rare but possible, errors. I think that
   this is legitimate behaviour, but it should be documented.

[albertz]

I think it is worth pointing out that the semantics of

f = ``open(fd, closefd=True)`` 

are broken (IMHO) because an exception can result in an unreferenced file
object that has taken over reponsibility for closing the fd, but it can
also fail without creating the file object.

I thought that if this raises a (normal) exception, it always means that it did not have overtaken the fd, i.e. never results in an unreferenced file object which has taken ownership of fd.

It this is not true right now, you are right that this is problematic. But this should be simple to fix on the CPython side, right? I.e. to make sure whenever some exception is raised here, even if some temporary file object already was constructed, it will not close fd. It should be consistent in this behavior, otherwise indeed, the semantics are broken.

[paulollis]

I thought that if this raises a (normal) exception, it always means that it
did not have overtaken the fd, i.e. never results in an unreferenced file
object which has taken ownership of fd.

The current CPython implementation does not guard against this happening. Some
incorrect combinations of arguments will definitely cause the problem. It is
also possible that it could occur under other circumstances.

It this is not true right now, you are right that this is problematic. But
this should be simple to fix on the CPython side, right? I.e. to make sure
whenever some exception is raised here, even if some temporary file object
already was constructed, it will not close fd. It should be consistent in
this behavior, otherwise indeed, the semantics are broken.

I agree. I think it should be fairly simple to fix for CPython.

[rouilj]

Just a heads up in python 3.11 on Travis CI I am seeing a similar issue with sqlite throwing an I/O error
randomly during testing. Some tests always fail, other tests randomly fail. E.g.

https://app.travis-ci.com/github/roundup-tracker/roundup/jobs/573649310#L5779
for a sample failure. Note that the same suite works fine with 3.8 and 3.10.

So 3.11 might be more prone to this. Does it make sense to add 3.11 to the labels on this ticket.

[serhiy-storchaka]

See #93874. It was mostly ready (except a NEWS entry and some polishing) more than 2 years ago. Now I do not remember why it was not finished at that time -- either I just put it off until the next morning, but was distracted by other issues and forgot about it, or I found some issues with it, or I found a better way. So please make a careful review.

Maybe it was deferred because there is a very tiny possibility of a file descriptor leak if you interrupt the program between os.open() and return. It is a part of more general issue and is not solvable at the moment.