tempfile.py rewrite

[zackw]

BPO	589982
Nosy	@gvanrossum, @jackjansen
Files	D: patch .vs. latest CVS mainline d.tempfile-rewrite-2: Revised patch.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gvanrossum'
closed_at = <Date 2002-08-18.06:52:24.000>
created_at = <Date 2002-08-02.06:38:20.000>
labels = ['library']
title = 'tempfile.py rewrite'
updated_at = <Date 2002-08-18.06:52:24.000>
user = 'https://bugs.python.org/zackw'

bugs.python.org fields:

activity = <Date 2002-08-18.06:52:24.000>
actor = 'aimacintyre'
assignee = 'gvanrossum'
closed = True
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2002-08-02.06:38:20.000>
creator = 'zackw'
dependencies = []
files = ['4465', '4466']
hgrepos = []
issue_num = 589982
keywords = ['patch']
message_count = 20.0
messages = ['40750', '40751', '40752', '40753', '40754', '40755', '40756', '40757', '40758', '40759', '40760', '40761', '40762', '40763', '40764', '40765', '40766', '40767', '40768', '40769']
nosy_count = 5.0
nosy_names = ['gvanrossum', 'nnorwitz', 'jackjansen', 'zackw', 'aimacintyre']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue589982'
versions = ['Python 2.3']

[zackw]

This rewrite closes a number of security-relevant races
in tempfile.py; makes temporary filenames much harder
to guess; provides secure interfaces that can be used
to close similar races elsewhere; and makes it possible
to control the prefix and directory of each temporary
created, individually.

[gvanrossum]

Logged In: YES
user_id=6380

This needs some serious review!

Volunteers???

[zackw]

Logged In: YES
user_id=580015

I've revised the patch; ignore the old one. This version
includes a vastly expanded test_tempfile.py which hits every
line that I know how to test. The omissions are marked -
it's mostly non-Unix issues.

Also, I went through the entire CVS repository and replaced
all uses of tempfile.mktemp with
mkstemp/mkdtemp/NamedTemporaryFile,
as appropriate. The sole exception is Lib/os.py, which is
addressed by patch bpo-590294.

The sole functional change to tempfile.py itself, from the
previous, is to throw os.O_NOFOLLOW into the open flags.
This closes yet another hole - on some systems, without this
flag, open(file, O_CREAT|O_EXCL) will follow a symbolic link
that points to a nonexistent file, and create the link
target. (This has no effect on a symlink in the directory
components of the pathname - if the sysadmin has symlinked
/tmp to /hugedisk/scratch, that still works.)

[gvanrossum]

Logged In: YES
user_id=6380

I like the idea of fixing security holes.

This patch is *humungous*. Even just the doc changes and the
changes to tempfile.py itself are massive and require very
careful reading to review all the consequences.

Zack, can you try to interest someone with more time than me
in reviewing this patch?

What's the point of renaming all imports with a leading
underscore? I thought __all__ took care of that.

[zackw]

Logged In: YES
user_id=580015

I'm afraid my idea of patch size comes from GCC land, where
this would be considered not that big at all. Only 1500
lines affected, and more than half of that is documentation
and test suite!

I tried, and failed, to break up the changes to tempfile.py
itself. But there's some larger divisions that could be
made. We could check in the new test_tempfile.py now,
disabling the tests that refer to nonexistent functions
(just comment out the lines that add those tests to the
test_classes array). The changes to the rest of the test
suite are also largely independent of the tempfile.py
rewrite (since they replace tempfile.mktemp() with TESTFN,
mostly). And the search-and-replace changes in the library
can wait until after tempfile.py itself gets reviewed.

Unfortunately, I am about to go on vacation for five days,
so I don't have time now to do this split-up. I will try
to drum up interest on python-dev in reviewing the patch as is.

[gvanrossum]

Logged In: YES
user_id=6380

OK, I'll do something along those lines myself.

[gvanrossum]

Logged In: YES
user_id=6380

I've checked this all in now. The changes to
test_tempfile.py weren't as easily fixable to work without
the tempfile.py changes as Zack thought.

I hope the community will give it some review. It will
probably break some Zope tests.

[nnorwitz]

Logged In: YES
user_id=33168

Guido, there are still 3 uses of mktemp after the checkin.
Should these use mkstemp()?

Lib/toaiff.py:102
Lib/plat-irix5/torgb.py:95
Lib/plat-irix6/torgb.py:95

[gvanrossum]

Logged In: YES
user_id=6380

Oops, looks like typos in the patch. Fixed (I hope).

Question for Zack: I noticed that a few times you changed this:

    temp = tempfile.mktemp()

into this:

(fd, temp) = tempfile.mkstemp()
os.close(fd)

If the latter is secure, why can't mktemp() be defined as
doing that?

[gvanrossum]

Logged In: YES
user_id=6380

I'm reopening this just as a precaution.

The snake farm reported two messages on HP-UX 11 when the
test suite was run:

Exception exceptions.AttributeError: "mkstemped instance has
no attribute 'fd'" in <bound method mkstemped.__del__ of
<test.test_tempfile.mkstemped instance at 0x407136e8>> ignored
Exception exceptions.AttributeError: "mkstemped instance has
no attribute 'fd'" in <bound method mkstemped.__del__ of
<test.test_tempfile.mkstemped instance at 0x40af9be8>> ignored

The mkstemped class is defined in test_maketemp.py. That
error can happen if a mkstemped instance isn't fully
initialized, e.g. if the _mkstemp_inner() call in
mkstemped.__init__ fails. But then I would have expected a
failure reported, which I don't see...

[gvanrossum]

Logged In: YES
user_id=6380

I'm reopening this just as a precaution.

The snake farm reported two messages on HP-UX 11 when the
test suite was run:

Exception exceptions.AttributeError: "mkstemped instance has
no attribute 'fd'" in <bound method mkstemped.__del__ of
<test.test_tempfile.mkstemped instance at 0x407136e8>> ignored
Exception exceptions.AttributeError: "mkstemped instance has
no attribute 'fd'" in <bound method mkstemped.__del__ of
<test.test_tempfile.mkstemped instance at 0x40af9be8>> ignored

The mkstemped class is defined in test_maketemp.py. That
error can happen if a mkstemped instance isn't fully
initialized, e.g. if the _mkstemp_inner() call in
mkstemped.__init__ fails. But then I would have expected a
failure reported, which I don't see...

[gvanrossum]

Logged In: YES
user_id=6380

I'd like to change the binary=True argument to mkstemp into
text=False; that seems easier to explain.

News about the HP errors from Kalle Svensson:

Traceback (most recent call last):
  File "../python/dist/src/Lib/test/test_tempfile.py", line
719, in ?
    test_main()
  File "../python/dist/src/Lib/test/test_tempfile.py", line
716, in test_main
    test_support.run_suite(suite)
  File
"/mp/slaskdisk/tmp/sfarmer/python/dist/src/Lib/test/test_support.py",
line 188, in run_suite
    raise TestFailed(err)
test.test_support.TestFailed: Traceback (most recent call last):
  File "../python/dist/src/Lib/test/test_tempfile.py", line
295, in test_basic_many
  File "../python/dist/src/Lib/test/test_tempfile.py", line
278, in do_create
  File "../python/dist/src/Lib/test/test_tempfile.py", line
33, in failOnException
  File
"/mp/slaskdisk/tmp/sfarmer/python/dist/src/Lib/unittest.py",
line 260, in fail
AssertionError: _mkstemp_inner raised exceptions.OSError:
[Errno 24] Too many open files: '/tmp/aaU3irrA'

[gvanrossum]

Logged In: YES
user_id=6380

The "Too many open files" problem is solved. The HP system
was configured to allow only 200 open file descriptors per
process.

But maybe the test would work just as well if it tried to
create 100 instead of 1000 temp files? I expect that this
would cause failures on other systems with conservative limits.

[gvanrossum]

Logged In: YES
user_id=6380

Closing again. Reduced the number of temp files to 100.
Changed 'binary=True' to 'text=False' default on mkstemp().

[jackjansen]

Logged In: YES
user_id=45365

Isn't it much more logical to give mkstemp() a mode="w+b" argument? The other routines have that as well, and it is also more in line with open() and such...

[jackjansen]

Logged In: YES
user_id=45365

Nevermind. Just saw the discussion on python-dev (this is a file descriptor returned, not a file pointer, so stdio is nowhere in sight).

[aimacintyre]

Logged In: YES
user_id=250749

I hope I'm doing the right thing by re-opening this rather than
copening a bug report...

I'm seeing a very odd failure in test_tempfile on FreeBSD 4.4.

The failure occurs when I run the full regression test with
TESTOPTS="-l -u network" but succeeds when TESTOPTS="-
l".

./python -E -tt Lib/test/regrtest.py -l -u network test_tempfile
succeeds too, as does:
./python -E -tt Lib/test/regrtest.py -v -u network test_tempfile

At this point, I haven't tried other -u option combinations.

The error log shows:
test test_tempfile failed -- Traceback (most recent call last):
  File "/home/andymac/cvs/python/python-
cvs/Lib/test/test_tempfile.py", line 345, in test_noinherit
    "child process exited successfully")
  File "/home/andymac/cvs/python/python-
cvs/Lib/unittest.py", line 268, in failUnless
    if not expr: raise self.failureException, msg
AssertionError: child process exited successfully 

Unfortunately Real Job has wiped out any time I might have
had to try and debug this, and I won't have much if any time
to look at this for about 3 weeks :-(

Intuitive guesses about where to start looking would be
welcome!

The OS/2 EMX port has the mkstemped problem noted below
for HP-UX, but I think I might not have picked up the fixes
when I tested, so I'll have to check that again.

[zackw]

Logged In: YES
user_id=580015

It sounds like some other test -- probably one of the ones
conditioned on -u network -- is causing the child process to
have a stale file descriptor open. Can you reproduce the
problem with

./python -E -tt ./Lib/test/regrtest.py -l -u network
test_socket_ssl test_socketserver test_tempfile

?

[gvanrossum]

Logged In: YES
user_id=6380

Andrew, can you check again with current CVS? I checked in
a fix to test_tempfile.py (and an auxiliary file
tf_inherit_check.py) that makes the failing test much more
robust (we hope).

[aimacintyre]

Logged In: YES
user_id=250749

I didn't get to try Zack's suggestion before my FreeBSD auto
build/test setup caught up with Guido's checkin.

With Guido's checkin, test_tempfile passes the TESTOPT="-l
-u network" test run that was previously failing.

OS/2 EMX actually had 2 problems:

• file/directory permissions behave like Windows, not Unix;
• EMX defaults to only 40 file handles.

I've checked in a small change to test_tempfile.py to deal
with the first issue (making it behave like Windows), and
checked in a Makefile change that ups the number of file
handles to 250. I've also added notes to the port README
about ways of overriding the number of file handles.

I'm closing this patch as my issues are now resolved.