Issue589982
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2002-08-02 06:38 by zackw, last changed 2022-04-10 16:05 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
D | zackw, 2002-08-02 06:38 | patch .vs. latest CVS mainline | ||
d.tempfile-rewrite-2 | zackw, 2002-08-03 06:53 | Revised patch. |
Messages (20) | |||
---|---|---|---|
msg40750 - (view) | Author: Zack Weinberg (zackw) | Date: 2002-08-02 06:38 | |
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. |
|||
msg40751 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-02 14:45 | |
Logged In: YES user_id=6380 This needs some serious review! Volunteers??? |
|||
msg40752 - (view) | Author: Zack Weinberg (zackw) | Date: 2002-08-03 06:53 | |
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.) |
|||
msg40753 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-05 16:48 | |
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. |
|||
msg40754 - (view) | Author: Zack Weinberg (zackw) | Date: 2002-08-08 19:01 | |
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. |
|||
msg40755 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-09 14:50 | |
Logged In: YES user_id=6380 OK, I'll do something along those lines myself. |
|||
msg40756 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-09 16:40 | |
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. |
|||
msg40757 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-09 20:53 | |
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 |
|||
msg40758 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-10 00:19 | |
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? |
|||
msg40759 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-10 04:17 | |
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... |
|||
msg40760 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-11 04:28 | |
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... |
|||
msg40761 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-14 13:02 | |
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' |
|||
msg40762 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-14 13:16 | |
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. |
|||
msg40763 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-14 16:52 | |
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(). |
|||
msg40764 - (view) | Author: Jack Jansen (jackjansen) * | Date: 2002-08-14 19:42 | |
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... |
|||
msg40765 - (view) | Author: Jack Jansen (jackjansen) * | Date: 2002-08-14 19:44 | |
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). |
|||
msg40766 - (view) | Author: Andrew I MacIntyre (aimacintyre) * | Date: 2002-08-16 09:00 | |
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. |
|||
msg40767 - (view) | Author: Zack Weinberg (zackw) | Date: 2002-08-16 18:12 | |
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 ? |
|||
msg40768 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-17 11:46 | |
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). |
|||
msg40769 - (view) | Author: Andrew I MacIntyre (aimacintyre) * | Date: 2002-08-18 06:52 | |
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. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-10 16:05:32 | admin | set | github: 36966 |
2002-08-02 06:38:20 | zackw | create |