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 2014年07月30日 12:40 by rupole, last changed 2022年04月11日 14:58 by admin.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| tempfile_bad_tempdir.patch | serhiy.storchaka, 2015年02月15日 09:03 | review | ||
| tempfile_bad_tempdir_2.patch | serhiy.storchaka, 2015年02月15日 19:03 | review | ||
| tempfile_bad_tempdir_3.patch | serhiy.storchaka, 2015年05月18日 18:41 | review | ||
| tempfile_bad_tempdir_4.patch | serhiy.storchaka, 2015年05月19日 17:26 | review | ||
| master...bjmcculloch_patch-1.diff | Billy McCulloch, 2016年05月04日 05:24 | fix PermissionError exception handlers | review | |
| Messages (43) | |||
|---|---|---|---|
| msg224304 - (view) | Author: Roger Upole (rupole) | Date: 2014年07月30日 12:40 | |
_mkstemp_inner assumes that an access denied error means that it has generated a filename that matches an existing foldername. However, in the case of a folder for which you don't have permissions to create a file, this means it will loop thru the maximum possible number of files. This causes it to hang for several seconds and eventually return a bogus FileExistsError. Similar behaviour exists in 2.7.7, but it throws an IOError instead. http://bugs.python.org/issue18849 seems to be where this was introduced. |
|||
| msg236004 - (view) | Author: Mark Lawrence (BreamoreBoy) * | Date: 2015年02月14日 22:51 | |
changeset 035b61b52caa has this:- return (fd, _os.path.abspath(file)) except FileExistsError: continue # try again + except PermissionError: + # This exception is thrown when a directory with the chosen name + # already exists on windows. + if _os.name == 'nt': + continue + else: + raise raise FileExistsError(_errno.EEXIST, "No usable temporary file name found") Could we simply set a flag saying it's a PermissionError and then raise the appropriate PermissionError or FileExistsError at the end of the loop? |
|||
| msg236028 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年02月15日 09:03 | |
What exceptions are raised on Windows when try to open a file in bad directory?
open('foo').close()
open('foo/bar') # what raised?
open('nonexistent/bar') # what raised?
If raised the same exceptions as on Linux, then perhaps the following patch fixes this issue.
|
|||
| msg236029 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年02月15日 09:13 | |
And what returns os.access for writable directories and non-existent files?
os.mkdir('dir')
os.access('dir', os.W_OK) # what returns?
os.access('nonexistent', os.W_OK) # what returns or raises?
|
|||
| msg236047 - (view) | Author: Mark Lawrence (BreamoreBoy) * | Date: 2015年02月15日 16:58 | |
>>> os.mkdir('dir')
>>> os.access('dir', os.W_OK)
True
>>> os.access('nonexistent', os.W_OK)
False
>>> open('dir/bar')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: 'dir/bar'
>>> open('nonexistent/bar')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: 'nonexistent/bar'
|
|||
| msg236050 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年02月15日 17:08 | |
Thank you Mark. Could you please make first test in msg236028 (when first part of the path is a file, not a directory)? |
|||
| msg236052 - (view) | Author: Mark Lawrence (BreamoreBoy) * | Date: 2015年02月15日 17:34 | |
>>> open('README').close()
>>> open('README/bar')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: 'README/bar'
|
|||
| msg236053 - (view) | Author: Steve Dower (steve.dower) * (Python committer) | Date: 2015年02月15日 17:36 | |
Is there a difference if you do open(..., 'w')? It's a different enough operation that it may have a different error. |
|||
| msg236055 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年02月15日 18:03 | |
> Is there a difference if you do open(..., 'w')? It's a different enough operation that it may have a different error.
Oh, yes, I forgot the 'w' mode.
Mark, could you please run following test on Windows?
import os
open('foo', 'wb').close()
flags = os.O_RDWR | os.O_CREAT | os.O_EXCL | getattr(os, 'O_NOFOLLOW', 0) | getattr(os, 'O_BINARY', 0)
os.open('foo/bar', flags, 0o600) # what raised?
os.open('nonexistent/bar', flags, 0o600) # what raised?
|
|||
| msg236058 - (view) | Author: Mark Lawrence (BreamoreBoy) * | Date: 2015年02月15日 18:44 | |
>>> open('foo', 'wb').close()
>>> flags = os.O_RDWR | os.O_CREAT | os.O_EXCL | getattr(os, 'O_NOFOLLOW', 0) | getattr(os, 'O_BINARY', 0)
>>> os.open('foo/bar', flags, 0o600)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: 'foo/bar'
>>> os.open('nonexistent/bar', flags, 0o600)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: 'nonexistent/bar'
|
|||
| msg236060 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年02月15日 19:03 | |
Great. There is only one difference between Windows and Linux, but it affects only error type in tests. Here is a patch with updated test. It should now work on Windows. |
|||
| msg236112 - (view) | Author: Roger Upole (rupole) | Date: 2015年02月16日 21:45 | |
os.access doesn't check filesystem permissions, so the patch will not catch the condition that creates the problem. |
|||
| msg236118 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年02月16日 22:52 | |
It is best that we can do. How else we can check filesystem permissions? Only trying to create a file, But we just tried this and it failed. |
|||
| msg236130 - (view) | Author: Roger Upole (rupole) | Date: 2015年02月17日 10:54 | |
It doesn't actually do anything, so why do it at all? In order to distinguish why it failed, you might try checking if the file actually exists, and if it is a folder. |
|||
| msg236132 - (view) | Author: Tim Golden (tim.golden) * (Python committer) | Date: 2015年02月17日 11:49 | |
And, just to be clear to Serhiy who I know doesn't use Windows, os.access really is a worthless function in its present form: worse, even, because it can be misleading. I have a long-standing patch to convert it to use AccessCheck but I've never quite had the guts to commit it because I fear the breakage would be too great. |
|||
| msg236133 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年02月17日 12:32 | |
The main issue is not tempfile raises a FileExistsError, but that it hangs for several seconds (for example if the temp dir doesn't exist). The patch allows to fail early and try other temp dir. os.access() is not enough, we can add os.path.isdir(). Could you please run tests on patched Python on Windows and say what tests are failed? |
|||
| msg236143 - (view) | Author: Mark Lawrence (BreamoreBoy) * | Date: 2015年02月17日 18:50 | |
The feedback here https://mail.python.org/pipermail/python-dev/2011-May/111530.html seems positive. It references #2528 which is still open but strikes me as the way forward. Why don't we go for it and nail this issue once and for all? |
|||
| msg243514 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年05月18日 18:41 | |
Added os.path.isdir(). Could anybody please run tests on Windows? |
|||
| msg243608 - (view) | Author: Paul Moore (paul.moore) * (Python committer) | Date: 2015年05月19日 16:47 | |
====================================================================== ERROR: test_read_only_directory (test.test_tempfile.TestMkdtemp) ---------------------------------------------------------------------- Traceback (most recent call last): File "C:\Work\Projects\cpython\lib\test\test_tempfile.py", line 267, in _inside_empty_temp_dir yield File "C:\Work\Projects\cpython\lib\test\test_tempfile.py", line 286, in test_read_only_directory self.skipTest("can't set the directory read-only") File "C:\Work\Projects\cpython\lib\unittest\case.py", line 645, in skipTest raise SkipTest(reason) unittest.case.SkipTest: can't set the directory read-only During handling of the above exception, another exception occurred: Traceback (most recent call last): File "C:\Work\Projects\cpython\lib\test\test_tempfile.py", line 289, in test_read_only_directory self.assertEqual(os.listdir(tempfile.tempdir), []) File "C:\Work\Projects\cpython\lib\contextlib.py", line 77, in __exit__ self.gen.throw(type, value, traceback) File "C:\Work\Projects\cpython\lib\test\test_tempfile.py", line 269, in _inside_empty_temp_dir support.rmtree(dir) File "C:\Work\Projects\cpython\lib\test\support\__init__.py", line 374, in rmtree _rmtree(path) File "C:\Work\Projects\cpython\lib\test\support\__init__.py", line 354, in _rmtree _waitfor(os.rmdir, path) File "C:\Work\Projects\cpython\lib\test\support\__init__.py", line 301, in _waitfor func(pathname) PermissionError: [WinError 5] Access is denied: 'C:\\Users\\Gustav\\AppData\\Local\\Temp\\tmpe53kiky0' ====================================================================== ERROR: test_read_only_directory (test.test_tempfile.TestMkstempInner) ---------------------------------------------------------------------- Traceback (most recent call last): File "C:\Work\Projects\cpython\lib\test\test_tempfile.py", line 267, in _inside_empty_temp_dir yield File "C:\Work\Projects\cpython\lib\test\test_tempfile.py", line 286, in test_read_only_directory self.skipTest("can't set the directory read-only") File "C:\Work\Projects\cpython\lib\unittest\case.py", line 645, in skipTest raise SkipTest(reason) unittest.case.SkipTest: can't set the directory read-only During handling of the above exception, another exception occurred: Traceback (most recent call last): File "C:\Work\Projects\cpython\lib\test\test_tempfile.py", line 289, in test_read_only_directory self.assertEqual(os.listdir(tempfile.tempdir), []) File "C:\Work\Projects\cpython\lib\contextlib.py", line 77, in __exit__ self.gen.throw(type, value, traceback) File "C:\Work\Projects\cpython\lib\test\test_tempfile.py", line 269, in _inside_empty_temp_dir support.rmtree(dir) File "C:\Work\Projects\cpython\lib\test\support\__init__.py", line 374, in rmtree _rmtree(path) File "C:\Work\Projects\cpython\lib\test\support\__init__.py", line 354, in _rmtree _waitfor(os.rmdir, path) File "C:\Work\Projects\cpython\lib\test\support\__init__.py", line 301, in _waitfor func(pathname) PermissionError: [WinError 5] Access is denied: 'C:\\Users\\Gustav\\AppData\\Local\\Temp\\tmp0qwkkr7l' |
|||
| msg243611 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年05月19日 17:26 | |
Thank you Paul. What with updated patch? |
|||
| msg243617 - (view) | Author: Paul Moore (paul.moore) * (Python committer) | Date: 2015年05月19日 18:13 | |
Works fine with the new patch: >.\rt.bat -x64 -q test_tempfile C:\Work\Projects\cpython\PCbuild>"C:\Work\Projects\cpython\PCbuild\amd64\python.exe" -Wd -E -bb "C:\Work\Projects\cpython\PCbuild\..\lib\test\regrtest.py" test_tempfile [1/1] test_tempfile 1 test OK. |
|||
| msg243626 - (view) | Author: Roundup Robot (python-dev) (Python triager) | Date: 2015年05月19日 21:14 | |
New changeset 63f0ae6e218a by Serhiy Storchaka in branch '2.7': Issue #22107: tempfile.gettempdir() and tempfile.mkdtemp() now try again https://hg.python.org/cpython/rev/63f0ae6e218a New changeset 3a387854d106 by Serhiy Storchaka in branch '3.4': Issue #22107: tempfile.gettempdir() and tempfile.mkdtemp() now try again https://hg.python.org/cpython/rev/3a387854d106 New changeset 1134198e23bd by Serhiy Storchaka in branch 'default': Issue #22107: tempfile.gettempdir() and tempfile.mkdtemp() now try again https://hg.python.org/cpython/rev/1134198e23bd |
|||
| msg243628 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年05月19日 21:24 | |
Unfortunately the patch doesn't fix original issue and looks as this can't be fixed until os.access will be more useful on Windows. But it fixes several related issues. mkstemp() now fails early if parent directory doesn't exist or is a file. gettempdir() and mkdtemp() now try again in case of collision on Windows as well as on Unix. I hope that fixing os.access will automatically fix original issue. Thanks Mark and Paul for testing on Windows. Thanks Tim for explaining issue with os.access. |
|||
| msg243787 - (view) | Author: Tim Golden (tim.golden) * (Python committer) | Date: 2015年05月21日 20:34 | |
My reluctance to commit the os.access patch is because it will cause such a behaviour change in a function which has been pretty stable for a long while. It'll certainly be more correct, but at the undoubted expense of breaking someone's long-working code. |
|||
| msg243788 - (view) | Author: Paul Moore (paul.moore) * (Python committer) | Date: 2015年05月21日 20:49 | |
I'm not sure I follow. Isn't the point of this patch to try again in certain cases of a PermissionError, where currently the code breaks out of the loop early? How can the result be worse than the current behaviour? Suerly sometimes (maybe not always) it works now where it previously failed, but that's a good thing? |
|||
| msg243805 - (view) | Author: Eryk Sun (eryksun) * (Python triager) | Date: 2015年05月22日 04:22 | |
Shouldn't it be checking whether `file` (or `filename`) is a directory [1]? For example: except PermissionError: # This exception is thrown when a directory with # the chosen name already exists on windows. if _os.name == 'nt' and _os.path.isdir(file): continue # If the directory allows write access, continue # trying names. On Windows, currently this test # doesn't work. The implementation assumes all # directories allow write access, but it really # depends on the directory's discretionary and # system access control lists (DACL & SACL). elif _os.access(dir, _os.W_OK): continue else: raise --- [1]: Windows sets the last error to ERROR_ACCESS_DENIED when a system call returns the NTSTATUS code STATUS_FILE_IS_A_DIRECTORY. This status code is returned by NtCreateFile and NtOpenFile when the target file is a directory, but the caller asked for anything but a directory (i.e. CreateOptions contains FILE_NON_DIRECTORY_FILE). |
|||
| msg243808 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2015年05月22日 05:48 | |
There is a risk of race condition. One process can create a directory `file`, then other process fails to create a file with the same name `file`, then the first process removes directory `file`, then the second process handles PermissionError. The same is possible also with threads. |
|||
| msg243812 - (view) | Author: Eryk Sun (eryksun) * (Python triager) | Date: 2015年05月22日 06:58 | |
Ok, I think I understand now. You chose an indirect check to avoid the race condition. If we have write access to the directory, then the PermissionError must be because a directory exists with the same name. Unfortunately os.access doesn't currently tell us this information. Moreover, checking `isdir(dir) and access(dir, W_OK)` is actually redundant as things currently stand. If `isdir` returns True that means GetFileAttributes didn't fail, which means `access` must return True as designed: return_value = (attr != INVALID_FILE_ATTRIBUTES) && ( !(mode & 2) || !(attr & FILE_ATTRIBUTE_READONLY) || (attr & FILE_ATTRIBUTE_DIRECTORY) ); |
|||
| msg243814 - (view) | Author: Eryk Sun (eryksun) * (Python triager) | Date: 2015年05月22日 07:16 | |
> My reluctance to commit the os.access patch is because it will > cause such a behaviour change in a function which has been > pretty stable for a long while. The original behavior could be preserved as the default. A keyword-only argument could enable checking access based on the file security and thread token. |
|||
| msg243815 - (view) | Author: Tim Golden (tim.golden) * (Python committer) | Date: 2015年05月22日 07:22 | |
That is a possibility which hadn't occurred to me. @eryksun, would you mind eyeballing the patch over on issue2582? It almost certainly won't apply cleanly as it's been almost two years since I last refreshed it, but you can hopefully gauge the intent. |
|||
| msg259559 - (view) | Author: Thomas Kluyver (takluyver) * | Date: 2016年02月04日 12:48 | |
This issue was closed, but I believe the original bug reported was not fixed: trying to create a temporary file in a directory where you don't have write permissions hangs for a long time before failing with a misleading FileExistsError, rather than failing immediately with PermissionError. I've just run into this on Python 3.5.1 while trying to use tempfile to check if a directory is writable - which I'm doing precisely because os.access() isn't useful on Windows! I find it hard to believe that there is no way to distinguish a failure because the name is already used for a subdirectory from a failure because we don't have permission to create a file. |
|||
| msg264779 - (view) | Author: Billy McCulloch (Billy McCulloch) * | Date: 2016年05月04日 05:24 | |
I've also run into this bug on Windows. In my case, the tempdir path includes directories on a network share, which I lack write access permissions to. Python tries to generate a *lot* of files, and never figures out it should move on to another directory. The attached patch for tempdir.py resolves my issue. In _get_default_tempdir() and _mkstemp_inner(), you want to know if the filename you tried to create already exists as a directory, not whether the parent directory is a directory – that's handled in _get_default_tempdir(). In mkdtemp(), attempting to create a directory with the same name as an existing directory does not throw a PermissionError, so the code is superfluous. |
|||
| msg264971 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2016年05月06日 12:45 | |
Billy, no, the parent directory is tested intentionally. If it isn't a directory, we should fail. May be there is a way to distinguish a failure because the name is already used for a subdirectory from a failure because we don't have permission to create a file, but I haven't access to Windows and can't experiment. Someone motivated and experienced should to do this work. |
|||
| msg265040 - (view) | Author: Eryk Sun (eryksun) * (Python triager) | Date: 2016年05月07日 03:49 | |
The Windows API loses information when mapping kernel status values to Windows error codes. For example, the following status values are all mapped to ERROR_ACCESS_DENIED:
STATUS_INVALID_LOCK_SEQUENCE 0xc000001e
STATUS_INVALID_VIEW_SIZE 0xc000001f
STATUS_ALREADY_COMMITTED 0xc0000021
STATUS_ACCESS_DENIED 0xc0000022
STATUS_PORT_CONNECTION_REFUSED 0xc0000041
STATUS_THREAD_IS_TERMINATING 0xc000004b
STATUS_DELETE_PENDING 0xc0000056
STATUS_FILE_IS_A_DIRECTORY 0xc00000ba
STATUS_FILE_RENAMED 0xc00000d5
STATUS_PROCESS_IS_TERMINATING 0xc000010a
STATUS_CANNOT_DELETE 0xc0000121
STATUS_FILE_DELETED 0xc0000123
Encrypting File System
STATUS_ENCRYPTION_FAILED 0xc000028a
STATUS_DECRYPTION_FAILED 0xc000028b
STATUS_NO_RECOVERY_POLICY 0xc000028d
STATUS_NO_EFS 0xc000028e
STATUS_WRONG_EFS 0xc000028f
STATUS_NO_USER_KEYS 0xc0000290
STATUS_ACCESS_DENIED is from a failed NtAccessCheck. STATUS_FILE_IS_A_DIRECTORY is from trying to open a directory as a file, because NT doesn't allow accessing the anonymous data stream of a directory, such as "dirname::$DATA", which is the same as trying to open "dirname" as a file. It only allows creating a named data stream for a directory, such as "dirname:streamname:$DATA".
The original status value may still be available, but only by calling the undocumented runtime library function, RtlGetLastNtStatus, which was added in XP (NT 5.1). After a failed system call, the Windows base API calls BaseSetLastNTError, which calls RtlNtStatusToDosError to get the Win32/DOS error code for a given NT status value. This in turn caches the last NT status in the LastStatusValue field of the thread environment block (TEB). RtlGetLastNtStatus gets this value from the TEB.
Possibly PyErr_SetExcFromWindowsErrWithFilenameObjects could capture the most recent kernel status value from RtlGetLastNtStatus(), to add this as a new "ntstatus" attribute of OSError. This wouldn't always be meaningful, since the thread's LastErrorValue (returned by GetLastError) isn't always related to a failed system call, but it can help in cases such as this, to distinguish a genuine denial of access from some other failure, and without suffering from race conditions.
For example, I added a "testdir" subdirectory to a directory and then modified the DACL of the parent directory to deny write/append access for all users. The following experiment checks the NT status using ctypes:
STATUS_ACCESS_DENIED = ctypes.c_long(0xC0000022).value
STATUS_FILE_IS_A_DIRECTORY = ctypes.c_long(0xC00000BA).value
ntdll = ctypes.WinDLL('ntdll')
try: open('testdir', 'w')
except: status_dir = ntdll.RtlGetLastNtStatus()
try: open('test', 'w')
except: status_access = ntdll.RtlGetLastNtStatus()
>>> status_dir == STATUS_FILE_IS_A_DIRECTORY
True
>>> status_access == STATUS_ACCESS_DENIED
True
Obviously using ctypes isn't recommended, since the status value needs to be captured ASAP after the call fails, so here's an example in C:
#define UNICODE
#include <Windows.h>
#include <fcntl.h>
#include <stdio.h>
typedef NTSTATUS (NTAPI *RTLGETLASTNTSTATUS)(VOID);
int main()
{
HMODULE hNtdll = GetModuleHandle(L"ntdll");
RTLGETLASTNTSTATUS RtlGetLastNtStatus = (RTLGETLASTNTSTATUS)
GetProcAddress(hNtdll, "RtlGetLastNtStatus");
if (_open("testdir", _O_CREAT | _O_EXCL) == -1)
printf("status_dir: %#08x\n", RtlGetLastNtStatus());
if (_open("test", _O_CREAT | _O_EXCL) == -1)
printf("status_access: %#08x\n", RtlGetLastNtStatus());
return 0;
}
output:
status_dir: 0xc00000ba
status_access: 0xc0000022
|
|||
| msg282403 - (view) | Author: Paul Hirsch (Paul Doom) | Date: 2016年12月05日 09:49 | |
Can the _mkstemp_inner portion of Billy McCulloch's patch be applied? Due to a large default os.TMP_MAX value (2147483647 - seems to be the current value on Win 7/8.1/10 I have access to), the following will push the CPU to 100% for a very long time when run under a non-elevated shell: -- import tempfile tempfile.TemporaryFile(dir='C:\Windows') ... wait ... -- In _mkstemp_inner() we should be testing for the filename, not parent directory here: except PermissionError: # This exception is thrown when a directory with the chosen name # already exists on windows. if (_os.name == 'nt' and _os.path.isdir(dir) and _os.access(dir, _os.W_OK)): continue Changing the _os.path.isdir(dir) call to _os.path.isdir(filename) is all that is needed to prevent the death loop and function correctly in cases where Windows os.access(dir, _os.W_OK) claims we have write access when we do not. |
|||
| msg285131 - (view) | Author: Rocco Matano (rocco.matano) | Date: 2017年01月10日 17:01 | |
I just experienced the described problem using Python 3.6.0 (Python 3.6.0 (v3.6.0:41df79263a11, Dec 23 2016, 08:06:12) [MSC v.1900 64 bit (AMD64)] on win32), but i do not understand the current status of this issue: On the one hand it is marked as 'open', which seems to imply that is still not resolved. On the other hand the resolution was set to 'fixed' in May 2015. Should i open a new issue for Python 3.6? |
|||
| msg337735 - (view) | Author: Jim Maloy (JDM) | Date: 2019年03月12日 11:57 | |
This issue persists as of today (March 2019), in Python 3.7.2 (64 bit) running on Windows 10. I gather from the comments that fixing it is no trivial matter, although I don't fully understand why. The hang of "several seconds" that was originally described is at least 30 seconds on that platform -- I'm not sure when it would clear, as I didn't have the patience to wait it out. https://stackoverflow.com/questions/55109076/python-tempfile-temporaryfile-hangs-on-windows-when-no-write-privilege It seems to me that a genuine naming collision would be pretty rare -- at least for the (fairly common) use case I'm dealing with, trying to check where the directory is writeable or not. Would it make sense to at least set a default number of collision-retries that is significantly lower? Say, between 3 and 10, instead of the system max? |
|||
| msg340738 - (view) | Author: Billy McCulloch (Billy McCulloch) * | Date: 2019年04月23日 18:07 | |
I stand by the patch file I previously submitted on 2016年05月04日. A more detailed analysis / description of my reasoning follows. Change 1 in _get_default_tempdir: A PermissionError is thrown on Windows if you attempt to create a file whose filename matches an existing directory. As the code currently stands, the `if` statement checks whether the proposed file's *parent* directory is a directory (which it is, or a FileNotFoundError would have been thrown), instead of whether the proposed filename conflicts with an existing directory. The edited expression is really a typo that, in the context of the code block, always evaluates `True`. Here’s what we’re now saying in the `if` block: when a PermissionError is raised, if we’re on Windows (the only currently supported platform to throw nonsense errors at us) AND the filename we chose simply conflicts with an existing directory AND we supposedly have write access to the parent directory, then we were just unlucky with the chosen name and should try again in the same parent directory. (I say supposedly because Windows seems to erroneously report True on this check, even when we don’t have write access. I wouldn’t be surprised if this last check does something useful in certain contexts, I just don’t know what they are.) Change 2 in _mkstemp_inner: Same as above for Change 1. While _get_default_tempdir uses this code block to make sure the system tempdir is really writable, and _mkstemp_inner does it so that a file descriptor can be returned, the result and arguments are the same. Change 3 in mkdtemp: For _get_default_tempdir and _mkstemp_inner, the blocks of code in question are creating temporary files. As such, they need to handle the oddball case for Windows where attempts to create a file with a filename which conflicts with an existing directory name result in a PermissionError. The same block of error handling code is copied in mkdtemp, even though this function never tries to create a file – it only tries to create a directory. As such, in the case that we try to create a directory with a name that already exists (whether as a file or a directory), we wouldn't be dealing with a PermissionError, we'd have a FileExistsError, which is already handled in mkdtemp by the preceding lines. The only way I’ve seen a PermissionError crop up for a call to mkdir on Windows is if the user doesn’t have permission to create filesystem objects in the parent directory. This is the intended usage of a PermissionError, so no special handling needed is required. Remember, a PermissionError shouldn’t happen if mkdtemp is called without a `dir` kwarg, because the _sanitize_params will invoke _get_default_tempdir, which will check to ensure that the parent directory is writable. As such, this block of code was superfluous, and the patch should not raise PermissionError in user code where it previously was caught. |
|||
| msg347076 - (view) | Author: Erik Aronesty (earonesty) * | Date: 2019年07月01日 21:12 | |
Series of operations needed to answer the questions os.access is not answering on windows:
bool CanAccessFolder( LPCTSTR folderName, DWORD genericAccessRights )
{
bool bRet = false;
DWORD length = 0;
if (!::GetFileSecurity( folderName, OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION
| DACL_SECURITY_INFORMATION, NULL, NULL, &length ) &&
ERROR_INSUFFICIENT_BUFFER == ::GetLastError()) {
PSECURITY_DESCRIPTOR security = static_cast< PSECURITY_DESCRIPTOR >( ::malloc( length ) );
if (security && ::GetFileSecurity( folderName, OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION
| DACL_SECURITY_INFORMATION, security, length, &length )) {
HANDLE hToken = NULL;
if (::OpenProcessToken( ::GetCurrentProcess(), TOKEN_IMPERSONATE | TOKEN_QUERY |
TOKEN_DUPLICATE | STANDARD_RIGHTS_READ, &hToken )) {
HANDLE hImpersonatedToken = NULL;
if (::DuplicateToken( hToken, SecurityImpersonation, &hImpersonatedToken )) {
GENERIC_MAPPING mapping = { 0xFFFFFFFF };
PRIVILEGE_SET privileges = { 0 };
DWORD grantedAccess = 0, privilegesLength = sizeof( privileges );
BOOL result = FALSE;
mapping.GenericRead = FILE_GENERIC_READ;
mapping.GenericWrite = FILE_GENERIC_WRITE;
mapping.GenericExecute = FILE_GENERIC_EXECUTE;
mapping.GenericAll = FILE_ALL_ACCESS;
::MapGenericMask( &genericAccessRights, &mapping );
if (::AccessCheck( security, hImpersonatedToken, genericAccessRights,
&mapping, &privileges, &privilegesLength, &grantedAccess, &result )) {
bRet = (result == TRUE);
}
::CloseHandle( hImpersonatedToken );
}
::CloseHandle( hToken );
}
::free( security );
}
}
return bRet;
}
|
|||
| msg347095 - (view) | Author: Eryk Sun (eryksun) * (Python triager) | Date: 2019年07月02日 00:24 | |
CanAccessFolder is incomplete for the following reasons: (1) It doesn't account for SeBackupPrivilege (read and execute access) and SeRestorePrivilege (write and delete access). If a create or open call requests backup semantics, these two privileges are checked by the I/O Manager in order to grant access before the request is sent to the filesystem device stack. That said, our os.open() call doesn't use O_OBTAIN_DIR (0x2000), an undocumented flag that allows opening a directory by requesting backup semantics. If it did use this undocumented flag, there would actually be no problem to solve since we would get FileExistsError instead of PermissionError. (2) It doesn't check the parent directory in case FILE_READ_ATTRIBUTES access (required for GENERIC_READ) or DELETE access (required for GENERIC_ALL) are either not granted or denied to the user for the target directory. These rights are granted by NT filesystem policy if FILE_READ_DATA and FILE_DELETE_CHILD access are granted to the parent directory, respectively. This is a general concern that's not relevant here since we only need to check the directory for write access (i.e. FILE_ADD_FILE). (3) It doesn't get the LABEL_SECURITY_INFORMATION from the file security, in order to check for no-read-up, no-write-up, and no-execute-up mandatory access. Adding files to a directory is disallowed if its mandatory label denies write-up access and its integrity level is higher than the caller's (e.g. the caller is a standard user at medium integrity level, and the directory is at high or system integrity level). (4) It doesn't check effective access via OpenThreadToken, in case the thread is impersonating. (5) It cannot take into account access granted or denied by filesystem filter drivers such as antimalware programs. For this, we need to actually try to open the directory with the requested access via CreateFile. We're granted the required access if CreateFile succeeds, or if it fails with a sharing violation (i.e. winerror 32). A sharing violation isn't an issue since in practice adding a file to a directory is internal to a filesystem; it doesn't count against shared data access. |
|||
| msg353353 - (view) | Author: And Clover (aclover) * | Date: 2019年09月27日 10:32 | |
Attempting to answer the question "did this open call fail because the path was a directory" by implication from "do we think we ought to be able to write a file to this directory" is IMO doomed. There's no reliable way to determine whether one should be able to write to a location, short of trying to write to it. There isn't in general and there especially isn't on Windows, for the reasons discussed by Eryk and more. I believe Billy's patch is an improvement over what we have, in as much as it's specifically checking for the condition we care about to work around a shortcoming of Windows error reporting. I would further remove the use of os.access, which does nothing useful (it reflects the read-only file attribute, which no-one uses, and which doesn't exist for directories anyway). Yes, there is a race condition if the directory goes away between use and check, but it seems vanishingly unlikely this could happen by accident, and it could only happen on purpose if an attacker can guess the random filenames in which case they already have worse attacks than just making mkstemp fail. In general failure is a much better outcome than hanging for hours on end. We may be overthinking this. Maybe it is OK to treat all permission errors as maybe-file-exists errors, like issue 18849 originally did, and just retry without trying to pick apart the entrails. ...just not quite as many as 2147483647 times. Given how unlikely an accidental filename clash is in the first place, I'm thinking a more realistic number of retries might be something like, 2. `os.TMP_MAX` probably isn't a good choice in any case, as it indicates the number of names C's tmpnam() can come up with, which (a) is unrelated to the number of names _RandomNameSequence can come up with and (b) we have no particular reason to try to completely exhaust anyway. |
|||
| msg353360 - (view) | Author: Erik Aronesty (earonesty) * | Date: 2019年09月27日 13:30 | |
i would like to point out that the primary reason any of this nonsense exists is because of short filename restrictions. i've replaces nearly all of my temp file creation code in all of my project to `return os.urandom(32).hex()` ... which is reliable secure, fast, and doesn't require any fileops sure, it doesn't work on 8.3 limited systems, but maybe the NamedTemp code should check that *first*.... and *if* it's OK to use long names... just use them, otherwise revert to existing behavior |
|||
| msg357075 - (view) | Author: Erik Aronesty (earonesty) * | Date: 2019年11月20日 14:05 | |
This is the fist of what I'm using: https://gist.github.com/earonesty/a052ce176e99d5a659472d0dab6ea361 Seems OK for my use cases. There's probably issues with relying on __del__ this way. But it solves the Windows close/reopen problem, too. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022年04月11日 14:58:06 | admin | set | github: 66305 |
| 2021年07月08日 18:35:20 | bugale bugale | set | nosy:
+ bugale bugale |
| 2021年03月04日 17:31:44 | eryksun | set | versions: + Python 3.9, Python 3.10, - Python 2.7, Python 3.5, Python 3.6, Python 3.7 |
| 2020年09月19日 19:01:59 | georg.brandl | set | nosy:
- georg.brandl |
| 2020年08月06日 19:13:13 | Jeffrey.Kintscher | set | nosy:
- Jeffrey.Kintscher |
| 2020年08月06日 13:15:45 | Václav Dvořák | set | nosy:
+ Václav Dvořák |
| 2020年05月14日 20:54:40 | Tor.Colvin | set | nosy:
+ Tor.Colvin |
| 2020年01月08日 05:57:39 | Jonathan Mills | set | versions: + Python 3.8 |
| 2020年01月08日 05:57:18 | Jonathan Mills | set | nosy:
+ Jonathan Mills |
| 2019年11月20日 14:05:42 | earonesty | set | messages: + msg357075 |
| 2019年09月27日 13:30:39 | earonesty | set | messages: + msg353360 |
| 2019年09月27日 10:32:28 | aclover | set | nosy:
+ aclover messages: + msg353353 |
| 2019年07月03日 21:11:21 | Jeffrey.Kintscher | set | nosy:
+ Jeffrey.Kintscher |
| 2019年07月02日 00:24:30 | eryksun | set | messages: + msg347095 |
| 2019年07月01日 21:30:09 | martin.panter | link | issue37477 superseder |
| 2019年07月01日 21:12:06 | earonesty | set | nosy:
+ earonesty messages: + msg347076 |
| 2019年04月23日 18:07:29 | Billy McCulloch | set | messages: + msg340738 |
| 2019年03月12日 11:57:14 | JDM | set | nosy:
+ JDM messages: + msg337735 |
| 2017年01月10日 17:12:25 | serhiy.storchaka | set | stage: resolved -> resolution: fixed -> components: + Library (Lib) versions: + Python 3.7, - Python 3.4 |
| 2017年01月10日 17:01:35 | rocco.matano | set | nosy:
+ rocco.matano messages: + msg285131 versions: + Python 3.6 |
| 2016年12月05日 09:49:19 | Paul Doom | set | type: behavior -> resource usage messages: + msg282403 nosy: + Paul Doom |
| 2016年05月07日 03:50:01 | eryksun | set | messages: + msg265040 |
| 2016年05月06日 12:45:25 | serhiy.storchaka | set | messages: + msg264971 |
| 2016年05月04日 05:24:35 | Billy McCulloch | set | files:
+ master...bjmcculloch_patch-1.diff nosy: + Billy McCulloch messages: + msg264779 |
| 2016年02月05日 19:41:50 | BreamoreBoy | set | nosy:
- BreamoreBoy |
| 2016年02月05日 19:12:12 | serhiy.storchaka | set | status: closed -> open |
| 2016年02月04日 12:48:54 | takluyver | set | nosy:
+ takluyver messages: + msg259559 |
| 2015年05月22日 07:22:32 | tim.golden | set | messages: + msg243815 |
| 2015年05月22日 07:16:42 | eryksun | set | messages: + msg243814 |
| 2015年05月22日 06:58:55 | eryksun | set | messages: + msg243812 |
| 2015年05月22日 05:48:34 | serhiy.storchaka | set | messages: + msg243808 |
| 2015年05月22日 04:22:37 | eryksun | set | nosy:
+ eryksun messages: + msg243805 |
| 2015年05月21日 20:49:26 | paul.moore | set | messages: + msg243788 |
| 2015年05月21日 20:34:53 | tim.golden | set | messages: + msg243787 |
| 2015年05月19日 21:24:32 | serhiy.storchaka | set | status: open -> closed resolution: fixed messages: + msg243628 stage: patch review -> resolved |
| 2015年05月19日 21:14:45 | python-dev | set | nosy:
+ python-dev messages: + msg243626 |
| 2015年05月19日 18:13:43 | paul.moore | set | messages: + msg243617 |
| 2015年05月19日 17:26:41 | serhiy.storchaka | set | files:
+ tempfile_bad_tempdir_4.patch messages: + msg243611 |
| 2015年05月19日 16:47:49 | paul.moore | set | nosy:
+ paul.moore messages: + msg243608 |
| 2015年05月18日 18:41:32 | serhiy.storchaka | set | files:
+ tempfile_bad_tempdir_3.patch messages: + msg243514 |
| 2015年02月17日 18:50:56 | BreamoreBoy | set | messages: + msg236143 |
| 2015年02月17日 12:32:34 | serhiy.storchaka | set | messages: + msg236133 |
| 2015年02月17日 11:49:49 | tim.golden | set | messages: + msg236132 |
| 2015年02月17日 10:54:58 | rupole | set | messages: + msg236130 |
| 2015年02月16日 22:52:51 | serhiy.storchaka | set | messages: + msg236118 |
| 2015年02月16日 21:45:15 | rupole | set | messages: + msg236112 |
| 2015年02月15日 19:03:52 | serhiy.storchaka | set | files:
+ tempfile_bad_tempdir_2.patch messages: + msg236060 |
| 2015年02月15日 18:44:58 | BreamoreBoy | set | messages: + msg236058 |
| 2015年02月15日 18:03:14 | serhiy.storchaka | set | messages: + msg236055 |
| 2015年02月15日 17:36:44 | steve.dower | set | messages: + msg236053 |
| 2015年02月15日 17:34:06 | BreamoreBoy | set | messages: + msg236052 |
| 2015年02月15日 17:08:59 | serhiy.storchaka | set | messages: + msg236050 |
| 2015年02月15日 16:58:13 | BreamoreBoy | set | messages: + msg236047 |
| 2015年02月15日 09:13:25 | serhiy.storchaka | set | messages: + msg236029 |
| 2015年02月15日 09:03:34 | serhiy.storchaka | set | files:
+ tempfile_bad_tempdir.patch nosy: + serhiy.storchaka messages: + msg236028 keywords: + patch stage: patch review |
| 2015年02月14日 22:51:27 | BreamoreBoy | set | nosy:
+ BreamoreBoy, tim.golden, zach.ware, steve.dower messages: + msg236004 |
| 2014年08月02日 07:29:41 | serhiy.storchaka | set | type: behavior versions: + Python 3.5 |
| 2014年08月01日 22:58:37 | terry.reedy | set | nosy:
+ georg.brandl, ncoghlan |
| 2014年07月30日 12:40:41 | rupole | create | |