classification
Title: the call of tempfile.NamedTemporaryFile fails and leaves a file on the disk
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Eugene Viktorov, SilentGhost, eryksun, martin.panter, python-dev, terry.reedy, vstinner
Priority: normal Keywords: patch

Created on 2016-02-18 16:17 by Eugene Viktorov, last changed 2016-02-29 19:17 by SilentGhost. This issue is now closed.

Files
File name Uploaded Description Edit
temp_file.py Eugene Viktorov, 2016-02-18 16:17
issue26385.diff SilentGhost, 2016-02-18 18:48 review
issue26385_2.diff SilentGhost, 2016-02-19 07:56 review
issue26385_3.diff SilentGhost, 2016-02-20 14:23 review
issue26385_4_py2.diff martin.panter, 2016-02-24 02:18 review
Messages (18)
msg260466 - (view) Author: Eugene Viktorov (Eugene Viktorov) Date: 2016-02-18 16:17
When calling the tempfile.NamedTemporaryFile with mode='wr' or with any other wrong value for "mode", it raises the ValueError and silently leaves an unknown, just created file on the file system.
msg260474 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-02-18 18:48
Here is a naïve fix including a test.
msg260493 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-19 01:09
This looks like an extension of Issue 21058. Does the unlink() work on Windows? It seems to me that the file is configured to remove itself on close(), therefore I expect unlink() will raise an exception of its own. Also made some suggestions in the code review.

This problem also affects Python 2, if you fudge the right wrong parameters:

>>> NamedTemporaryFile((), prefix="blaua.")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/proj/python/cpython/Lib/tempfile.py", line 477, in NamedTemporaryFile
    file = _os.fdopen(fd, mode, bufsize)
TypeError: argument 2 must be string, not tuple
[59140 refs]
>>> glob("/tmp/blaua.*")
['/tmp/blaua.AFtEqx']
msg260505 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-02-19 04:54
> Does the unlink() work on Windows?

Yes. O_TEMPORARY opens the file with FILE_SHARE_DELETE, so unlink won't raise an error. 

Opening a file creates and returns a handle for a kernel File object that references the underlying file/link/stream control block in the file system. There may be multiple open File objects from separate NtCreateFile and NtOpenFile system calls, but they all reference a common file control block. Deleting a file requires a File handle, which is used to set the delete disposition in the control block. When all references (handle and pointer) to all File objects that reference the file are closed, the file is unlinked if the delete disposition is set.

The way delete-on-close works is to set a flag in the File object that causes the delete disposition to be automatically set when the File object is closed. However, any File handle that references the file can be used to set or unset this disposition if it has DELETE access. So it's harmless to manually call DeleteFile on the file (i.e. NtOpenFile and NtSetInformationFile to set the delete disposition) beforehand.
msg260506 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-02-19 07:56
I wonder if Victor could clarify why bare except wasn't used in the python3 version.

Anyway, here is the updated patch testing for TypeError as well.
msg260510 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-02-19 08:30
> I wonder if Victor could clarify why bare except wasn't used in the python3 version.

What do you call a "bare except"?

I wrote "except Exception: <cleanup code>; raise", it doesn't ignore the error. I want to always call the cleanup code on error.
msg260511 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-19 08:49
Victor: You changed “except Exception:” to bare “except:” in revision 182f08c0dd45 in Python 2, but the Python 3 code never got such a change.
msg260512 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-02-19 09:03
> Victor: You changed “except Exception:” to bare “except:” in revision 182f08c0dd45 in Python 2, but the Python 3 code never got such a change.

Ah strange :-) Python 3 must also be fixed.
msg260549 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-02-20 02:40
`except:` is equivalent to `except BaseException:`.  When I introduced the former into idlelib, a couple of years ago, I was told to do the latter in a follow-up patch, so that there would be no doubt as to the intent.  The same should be done here.
msg260551 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-02-20 03:05
I prefer "except:" over "except BaseException:". What is the benefit
of passing explicitly BaseException?
msg260555 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-20 03:55
Eryk Sun: The patch proposes to add an unlink() call after the file has been closed:

except Exception:
    _os.close(fd)  # This automatically deletes the file right?
    _os.unlink(name)  # Won’t this raise FileNotFoundError?
    raise

By your explanation, it sounds like it would be better to call unlink() before close().

Terry & Victor: Writing the explicit “except BaseException:” makes it clear you weren’t being lazy in figuring out what exceptions you want to catch. But in this case the “raise” at the end of the the exception handler make it clear enough for me. I would be happy with either option.
msg260557 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-02-20 04:05
It makes it clearer that you know that it will 'everything' and intend to catch do so and that you did not casually toss in 'except:', as used to be the habit of many.  There are over 20 bare excepts in idlelib and I suspect many or most are unintentionally over-broad.
msg260565 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-02-20 14:00
> By your explanation, it sounds like it would be better 
> to call unlink() before close().

Sorry, I was responding in general, because I thought you meant unlink would fail like it would for most open files on Windows, because the CRT normally doesn't open files with delete sharing. But I see what you meant now. Yes, the order needs to be reversed as unlink() and then close() for this to work. Doing the close first does raise a FileNotFoundError.
msg260566 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-02-20 14:23
Here is the updated patch including fixes for except and order of deletion.
msg260578 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-20 21:57
I think patch 3 is good for Python 3, thankyou. For Python 2, the test will have to be adjusted. From memory, mode='wr' is accepted without an exception, and mode=2 triggers an early error (so we never trigger the bug).
msg260598 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-02-21 08:28
Then, I think the TypeError check could be dropped and 'wr' replaced by an obviously wrong value, both seem fairly trivial. I don't have a working 2.7 checkout, so if anyone wants to extend the fix to that branch, they're more than welcome.
msg260758 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-24 02:18
Here is my proposed version for Python 2.
msg261004 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-29 11:28
New changeset 5bfb4147405e by Martin Panter in branch '2.7':
Issue #26385: Cleanup NamedTemporaryFile if fdopen() fails, by SilentGhost
https://hg.python.org/cpython/rev/5bfb4147405e

New changeset a1c125f21db4 by Martin Panter in branch '3.5':
Issue #26385: Cleanup NamedTemporaryFile if open() fails, by SilentGhost
https://hg.python.org/cpython/rev/a1c125f21db4

New changeset 865cf8eba51a by Martin Panter in branch 'default':
Issue #26385: Merge NamedTemporaryFile fix from 3.5
https://hg.python.org/cpython/rev/865cf8eba51a
History
Date User Action Args
2016-02-29 19:17:40SilentGhostsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-02-29 11:28:38python-devsetnosy: + python-dev
messages: + msg261004
2016-02-24 02:18:11martin.pantersetfiles: + issue26385_4_py2.diff

messages: + msg260758
2016-02-21 08:28:09SilentGhostsetmessages: + msg260598
2016-02-20 21:57:01martin.pantersetmessages: + msg260578
2016-02-20 14:23:43SilentGhostsetfiles: + issue26385_3.diff

messages: + msg260566
2016-02-20 14:00:46eryksunsetmessages: + msg260565
2016-02-20 04:05:46terry.reedysetmessages: + msg260557
2016-02-20 03:55:50martin.pantersetmessages: + msg260555
2016-02-20 03:05:17vstinnersetmessages: + msg260551
2016-02-20 02:40:26terry.reedysetnosy: + terry.reedy
messages: + msg260549
2016-02-19 09:11:41georg.brandlsetnosy: - georg.brandl
2016-02-19 09:03:11vstinnersetmessages: + msg260512
2016-02-19 08:49:48martin.pantersetmessages: + msg260511
2016-02-19 08:30:12vstinnersetmessages: + msg260510
2016-02-19 07:56:09SilentGhostsetfiles: + issue26385_2.diff
nosy: + vstinner
messages: + msg260506

2016-02-19 04:54:15eryksunsetnosy: + eryksun
messages: + msg260505
2016-02-19 01:09:48martin.pantersetnosy: + martin.panter

messages: + msg260493
versions: + Python 2.7, - Python 3.4
2016-02-18 18:48:04SilentGhostsetfiles: + issue26385.diff

versions: + Python 3.5, Python 3.6
keywords: + patch
nosy: + georg.brandl, SilentGhost

messages: + msg260474
stage: patch review
2016-02-18 16:17:52Eugene Viktorovcreate