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

[Eryk Sun]

Change by Eryk Sun :


--
versions: +Python 3.10 -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Eryk Sun]

Change by Eryk Sun :


--
Removed message: https://bugs.python.org/msg359981

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Jakub Stasiak]

Change by Jakub Stasiak :


--
nosy: +jstasiak

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Paul Ollis]

Paul Ollis  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Albert Zeyer]

Albert Zeyer  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[rekcartgubnohtyp]

Change by rekcartgubnohtyp :


--
nosy: +rekcartgubnohtyp

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Paul Ollis]

Paul Ollis  added the comment:

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.

--
nosy: +paul_ollis

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Albert Zeyer]

Albert Zeyer  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Robert Xiao]

Robert Xiao  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Eryk Sun]

Eryk Sun  added the comment:

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

os.remove will succeed on a file that's opened with O_TEMPORARY, which shares 
delete access (i.e. FILE_SHARE_DELETE).

With classic Windows delete semantics, deleting a file sets the delete 
disposition on the filesystem's underlying file/link control block (FCB/LCB). 
The filesystem doesn't unlink the file from the directory until all File object 
references have been finalized. (A File handle refers to a kernel File object, 
which refers to an FCB/LCB in a filesystem. An FCB/LCB can be referenced by 
multiple File objects, such as from multiple CreateFileW calls, and a File 
object can be referenced by multiple handles, such as via inheritance or 
DuplicateHandle.) The deleted file remains accessible to existing File objects, 
and a File object with delete access can even be used to clear the delete 
disposition (i.e. undelete the file) via SetFileInformationByHandle: 
FileDispositionInfo. The file remains linked and visible in the parent 
directory, but no new access is allowed while its delete disposition is set. 
The parent directory cannot itself be deleted until the file is unlinked.

In Windows 10 1903, DeleteFileW has also been updated to use POSIX semantics if 
the filesystem supports it, which includes NTFS on the system drive, where temp 
files are usually created. With POSIX semantics, the file is unlinked as soon 
as DeleteFileW closes its File handle. All existing File objects can continue 
to access the file even though it's no longer linked in the directory. This 
includes being able to call GetFinalPathNameByHandleW, which, at least for 
NTFS, will show that the file has been moved to the special system directory 
"\$Extend\$Deleted" and renamed according to its file ID. As is usual for a 
deleted file, no new access is allowed, i.e. we cannot reopen a file in the 
"$Deleted" directory. Another change with POSIX semantics is that the delete is 
final. Existing File objects that have delete access are not allowed to clear 
the delete disposition (i.e. undelete the file).

--
nosy: +eryksun

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Albert Zeyer]

Albert Zeyer  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Carl Harris]

Change by Carl Harris :


--
nosy: +hitbox

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Jörn Heissler]

Change by Jörn Heissler :


--
nosy: +joernheissler

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ned Batchelder]

Change by Ned Batchelder :


--
keywords: +patch
pull_requests: +17389
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/17985

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ned Batchelder]

Change by Ned Batchelder :


--
nosy: +nedbat

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
assignee:  -> serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Fabio Sangiovanni]

Change by Fabio Sangiovanni :


--
nosy: +sanjioh

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Albert Zeyer]

Albert Zeyer  added the comment:

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

--
nosy: +Albert.Zeyer

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Seth Troisi]

Change by Seth Troisi :


--
nosy: +Seth.Troisi

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
nosy: +serhiy.storchaka
versions:  -Python 3.5, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Robert Xiao]

New submission from Robert Xiao :

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.

--
components: Library (Lib)
messages: 359888
nosy: nneonneo
priority: normal
severity: normal
status: open
title: NamedTemporaryFile could cause double-close on an fd if 
_TemporaryFileWrapper throws
type: behavior
versions: Python 3.5, Python 3.6, Python 3.7, Python 3.8, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com