[Patches] [ python-Patches-589982 ] tempfile.py rewrite

noreply@sourceforge.net noreply@sourceforge.net
Wed, 14 Aug 2002 12:42:28 -0700


Patches item #589982, was opened at 2002-08-02 08:38
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=589982&group_id=5470

Category: Library (Lib)
Group: Python 2.3
Status: Closed
Resolution: Accepted
Priority: 5
Submitted By: Zack Weinberg (zackw)
Assigned to: Guido van Rossum (gvanrossum)
Summary: tempfile.py rewrite

Initial Comment:
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.

----------------------------------------------------------------------

>Comment By: Jack Jansen (jackjansen)
Date: 2002-08-14 21:42

Message:
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...

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-14 18:52

Message:
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().

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-14 15:16

Message:
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.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-14 15:02

Message:
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'


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-11 06:28

Message:
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...

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-10 06:17

Message:
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...

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-10 02:19

Message:
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?

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2002-08-09 22:53

Message:
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

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-09 18:40

Message:
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.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-09 16:50

Message:
Logged In: YES 
user_id=6380

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

----------------------------------------------------------------------

Comment By: Zack Weinberg (zackw)
Date: 2002-08-08 21:01

Message:
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.



----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-05 18:48

Message:
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.

----------------------------------------------------------------------

Comment By: Zack Weinberg (zackw)
Date: 2002-08-03 08:53

Message:
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 #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.)


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-02 16:45

Message:
Logged In: YES 
user_id=6380

This needs some serious review!

Volunteers???

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=589982&group_id=5470