Permalink
Browse files

Fix bug where we would overwrite existing file descriptors with dup2().

Now we check file descriptors (even those above #10) before using them.

This is a recurrence of the bug described here:

http://www.oilshell.org/blog/2017/07/02.html

If you do 'import random' in Python, it results in /dev/urandom being
permanently open (which can be seen in the blog post!)

At the time, I incorrectly fixed the bug by just opening our own
descriptors above #10.  But it's possible that CPython will subsequently
open something above #10 (/dev/urandom appeared as #12), so we have to
additionally check descriptors before we use them.

NOTE: 'import cgi' eventually causes 'import random'.  We're only using
it for cgi.escape(), but cgi.FieldStorage() uses tempfile, which uses a
random number generator.

Also: minimize the test case as gold/configure-bug.sh.

'test/gold.sh configure' was the case that caught this bug.  The spec
tests didn't find it!  Because it appears to take about 4 redirects for
this to happen!
  • Loading branch information...
Andy Chu
Andy Chu committed Sep 3, 2018
1 parent 41c2121 commit 94c7d7819dbf807fd0fae4b1a5e744779f84f286
Showing with 36 additions and 8 deletions.
  1. +22 −8 core/process.py
  2. +13 −0 gold/configure-bug.sh
  3. +1 −0 test/gold.sh
View
@@ -51,6 +51,18 @@ def __init__(self, next_fd=10):
self.cur_frame = _FdFrame() # for the top level
self.stack = [self.cur_frame]
def _NextFreeFileDescriptor(self):
done = False
while not done:
try:
fcntl.fcntl(self.next_fd, fcntl.F_GETFD)
except IOError as e:
if e.errno == errno.EBADF:
done = True
self.next_fd += 1
return self.next_fd
def Open(self, path):
"""Opens a path for read, but moves it out of the reserved 3-9 fd range.
@@ -61,9 +73,8 @@ def Open(self, path):
OSError if the path can't be found.
"""
fd = os.open(path, os.O_RDONLY, 0666)
new_fd = self.next_fd
new_fd = self._NextFreeFileDescriptor()
os.dup2(fd, new_fd)
self.next_fd += 1
os.close(fd)
return os.fdopen(new_fd)
@@ -75,11 +86,12 @@ def _PushDup(self, fd1, fd2):
Returns:
success Bool
"""
new_fd = self._NextFreeFileDescriptor()
#log('---- _PushDup %s %s', fd1, fd2)
need_restore = True
try:
#log('DUPFD %s %s', fd2, self.next_fd)
fcntl.fcntl(fd2, fcntl.F_DUPFD, self.next_fd)
fcntl.fcntl(fd2, fcntl.F_DUPFD, new_fd)
except IOError as e:
# Example program that causes this error: exec 4>&1. Descriptor 4 isn't
# open.
@@ -91,7 +103,7 @@ def _PushDup(self, fd1, fd2):
raise
else:
os.close(fd2)
fcntl.fcntl(self.next_fd, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
fcntl.fcntl(new_fd, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
#log('==== dup %s %s\n' % (fd1, fd2))
try:
@@ -100,14 +112,13 @@ def _PushDup(self, fd1, fd2):
# bash/dash give this error too, e.g. for 'echo hi 1>&3'
util.error('%d: %s', fd1, os.strerror(e.errno))
# Restore and return error
os.dup2(self.next_fd, fd2)
os.close(self.next_fd)
os.dup2(new_fd, fd2)
os.close(new_fd)
# Undo it
return False
if need_restore:
self.cur_frame.saved.append((self.next_fd, fd2))
self.next_fd += 1
self.cur_frame.saved.append((new_fd, fd2))
return True
def _PushClose(self, fd):
@@ -249,6 +260,9 @@ def Pop(self):
raise
os.close(saved)
#log('dup2 %s %s', saved, orig)
# NOTE: This balances the increments from _PushDup(). But it doesn't
# balance the ones from Open().
self.next_fd -= 1 # Count down
for fd in frame.need_close:
View
@@ -0,0 +1,13 @@
#!/bin/bash
detect_readline() {
echo foo >/dev/null 2>&1
echo "two" 1>&2
}
main() {
detect_readline > _tmp/f2-out.txt
#detect_readline
}
main "$@"
View
@@ -88,6 +88,7 @@ startup-benchmark() {
}
configure() { _compare ./configure; }
configure-bug() { _compare gold/configure-bug.sh; }
nix() { _compare gold/nix.sh isElfSimpleWithStdin; }
and-or() { _compare gold/and-or.sh test-simple; }

0 comments on commit 94c7d78

Please sign in to comment.