classification
Title: NamedTemporaryFile with delete=True should not fail if file already deleted
Type: behavior Stage:
Components: IO Versions: Python 3.6, Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: andrewnester, christian.heimes, jwilk, martin.panter, r.david.murray, rhettinger, richardxia
Priority: normal Keywords:

Created on 2017-02-15 21:21 by richardxia, last changed 2017-03-27 22:58 by martin.panter.

Pull Requests
URL Status Linked Edit
PR 134 open andrewnester, 2017-02-16 13:57
Messages (14)
msg287888 - (view) Author: Richard Xia (richardxia) Date: 2017-02-15 21:21
Here is a very short program to demonstrate what I'm seeing:

>>> import tempfile
>>> import os
>>> with tempfile.NamedTemporaryFile(delete=True) as fp:
...     print(fp.name)
...     os.system('rm {}'.format(fp.name))
/tmp/tmpomw0udc6
0
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "/usr/local/lib/python3.6/tempfile.py", line 502, in __exit__
    self.close()
  File "/usr/local/lib/python3.6/tempfile.py", line 509, in close
    self._closer.close()
  File "/usr/local/lib/python3.6/tempfile.py", line 446, in close
    unlink(self.name)
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpomw0udc6'


In my specific use case, I am shelling out to another program (binutils' objcopy) and passing the path of the NamedTemporaryFile as the output file. objcopy apparently deletes the output file if it encounters an error, which causes NamedTemporaryFile's exit method to fail when it tries to delete the file.

While I can work around this by using delete=False and manually doing the cleanup on my own, it's less elegant than being able to rely on the normal context manager exit.
msg287946 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-02-16 13:58
I've just added PR fixing this issue.
msg288156 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-02-19 19:19
any updates on this? :)
msg288612 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-02-26 21:04
some updates?
msg288686 - (view) Author: Jakub Wilk (jwilk) Date: 2017-02-28 00:46
Deleting non-existent files in /tmp is an indicator of a security hole.
This kind of error must never pass silently.
msg290083 - (view) Author: Andrew Nester (andrewnester) * Date: 2017-03-24 11:27
any updates?
msg290144 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-03-24 21:23
> Deleting non-existent files in /tmp is an indicator 
> of a security hole.

Christian, what is your take on this?
msg290642 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-03-27 20:27
Raymond, I think Richard's approach is problematic at best. 

Richard, you cannot use a NamedTempFile with an external process like that. At least you have to flush the file to disk. Even flushing is not safely portable. The safest and most portable approach is to close the file, call the other process and then unlink the file manually:

with tempfile.NamedTemporaryFile(delete=False) as f:
    try:    
        f.write(b'data')
        f.close()
        subprocess.Popen(['binutil', f.name, ...])
    finally:
        os.unlink(f.name)


It's too bad that close() on a NamedTemporaryFile(delete=True) deletes the files. For your problem it would be beneficial to have __exit__() perform the unlink operation.
msg290650 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-03-27 21:02
He's still going to get an error using your code, Christian.  But if he knows that the file being gone is OK, he can catch and ignore the error.  Having exit do the unlink wouldn't help him; in that case he'd have to wrap the whole 'with' clause in a try/except.

So I think delete=False and handling the error "manually" is the correct solution here, since I doubt we want to add a "already_deleted_ok=False" flag to the API for this limited use case that can be handled with delete=False.

I vote for closing this as rejected.
msg290651 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-03-27 21:05
RDM, I don't see how my example is going to fail. It uses delete=False with an explicit unlink. Please educate me. :)
msg290656 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-03-27 21:20
His problem is that the file has already been deleted by the time the subprocess returns, so the unlink is going to raise an exception.
msg290658 - (view) Author: Richard Xia (richardxia) Date: 2017-03-27 21:34
Thanks for the discussion. I ended up doing something similar to the code snippet Christian posted, except I also had a second try/except FileNotFoundError within the original finally block to catch the case that David pointed out.

In retrospect, I probably should have used TemporaryDirectory since I am using Python 3.5 and because the file I was creating with NamedTemporaryFile was only being used as an output file, not an input file, to the subprocess command.
msg290661 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-03-27 21:50
Ah! I missed the fact that the subprocess removes the file. It's easily fixable, wrap the unlink and catch FileNotFoundError / ENOENT.
msg290665 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-03-27 22:58
See also Issue 27425, about making the deletion step more flexible after the file has been created, which might help here.

I’m not sure about security problems, but IMO failure to remove a temporary file (because it is already gone, or some other reason) is likely to be a programming mistake, so I would not want it silenced.
History
Date User Action Args
2017-03-27 22:58:11martin.pantersetnosy: + martin.panter
messages: + msg290665
2017-03-27 21:50:00christian.heimessetmessages: + msg290661
2017-03-27 21:34:41richardxiasetmessages: + msg290658
2017-03-27 21:20:22r.david.murraysetmessages: + msg290656
2017-03-27 21:05:01christian.heimessetmessages: + msg290651
2017-03-27 21:02:40r.david.murraysetnosy: + r.david.murray
messages: + msg290650
2017-03-27 20:27:37christian.heimessetmessages: + msg290642
2017-03-24 21:23:01rhettingersetnosy: + christian.heimes, rhettinger
messages: + msg290144
2017-03-24 11:27:48andrewnestersetmessages: + msg290083
2017-02-28 00:46:23jwilksetnosy: + jwilk
messages: + msg288686
2017-02-26 21:04:16andrewnestersetmessages: + msg288612
2017-02-19 19:19:35andrewnestersetmessages: + msg288156
2017-02-16 13:58:04andrewnestersetnosy: + andrewnester
messages: + msg287946
2017-02-16 13:57:47andrewnestersetpull_requests: + pull_request96
2017-02-15 21:21:19richardxiacreate