tempfile module misinterprets access denied error on Windows #66305
Comments
|
_mkstemp_inner assumes that an access denied error means that it 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. |
|
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? |
|
What exceptions are raised on Windows when try to open a file in bad directory? open('foo').close() If raised the same exceptions as on Linux, then perhaps the following patch fixes this issue. |
|
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? |
>>> 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' |
|
Thank you Mark. Could you please make first test in msg236028 (when first part of the path is a file, not a directory)? |
>>> 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' |
|
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? |
>>> 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' |
|
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. |
|
os.access doesn't check filesystem permissions, so the patch will not catch the condition that creates the problem. |
|
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. |
|
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. |
|
And, just to be clear to Serhiy who I know doesn't use Windows, |
|
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? |
|
The feedback here https://mail.python.org/pipermail/python-dev/2011-May/111530.html seems positive. It references bpo-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? |
|
Added os.path.isdir(). Could anybody please run tests on Windows? |
|
====================================================================== 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'====================================================================== 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' |
|
Thank you Paul. What with updated patch? |
|
Works fine with the new patch:
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 |
|
New changeset 63f0ae6e218a by Serhiy Storchaka in branch '2.7': New changeset 3a387854d106 by Serhiy Storchaka in branch '3.4': New changeset 1134198e23bd by Serhiy Storchaka in branch 'default': |
|
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. |
|
My reluctance to commit the os.access patch is because it will cause |
|
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? |
|
Shouldn't it be checking whether 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 |
|
There is a risk of race condition. One process can create a directory |
|
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. 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? |
|
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: Change 2 in _mkstemp_inner: Change 3 in mkdtemp: |
|
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;
} |
|
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. |
|
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 bpo-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.
|
|
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 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 |
|
This is the fist of what I'm using: 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. |
eryksun
added
3.9
iritkatriel
added
3.11
|
I can't believe this issue haven't been solved after ten years. And some package really takes tempfile.TemporaryFile() for writability check. Hope Windows users enjoy the 2147483647 loops. |
|
Maybe there really is a reason for PathYetAnotherMakeUniqueName? |
|
Ran into this issue yesterday when I tried using a module that used Numba on Windows. Somebody has probably already mentioned this, but I'm not reading this whole thread. The root of the problem is this comment in posixmodule.c:
This is NOT true. While there is no exact (single) equivalent, Windows has multiple permissions that make directories effectively read-only. In this case the permission that is lacking is FILE_ADD_FILE, which allows files to be created in that directory (an alias of FILE_WRITE_DATA, which is the effect it has when applied to files). Related is FILE_ADD_SUBDIRECTORY (an alias of FILE_APPEND_DATA). After a bit of searching, I found a straightforward solution in a 2004 (cough 10 years before the problem appeared in Python cough) post by old Chen. Here is a hacky workaround I made for my own use. It probably isn't sufficiently robust for production code, but it gets the job done until Python properly fixes it: |
|
Changing I'm not sure whether it would unblock the original issue, but only because I also haven't re-read the entire thread. |
|
For an official fix it would probably be best to make I can confirm that at least in the case of Numba (which used tempfile to create temporary files) my workaround does in fact fix the issue with _mkstemp_inner (the topic of this issue). The attempt to create a temp file in a non-writable directory fails immediately for access denied, and Numba falls back to using the user's temp directory (the proper solution). |
|
I have just encountered this issue, when huggingface_hub tried to create a temporary dir in I also tested def mkdtemp(suffix=None, prefix=None, dir=None):
"""User-callable function to create and return a unique temporary
directory. The return value is the pathname of the directory.
Arguments are as for mkstemp, except that the 'text' argument is
not accepted.
The directory is readable, writable, and searchable only by the
creating user.
Caller is responsible for deleting the directory when done with it.
"""
prefix, suffix, dir, output_type = _sanitize_params(prefix, suffix, dir)
if not _os.path.exists(dir): raise FileNotFoundError(_errno.ENOENT, "No such file or directory: %r" % dir)
if not _os.path.isdir(dir): raise NotADirectoryError(_errno.ENOTDIR, "Not a directory: %r" % dir)
names = _get_candidate_names()
if output_type is bytes:
names = map(_os.fsencode, names)
for seq in range(TMP_MAX):
name = next(names)
file = _os.path.join(dir, prefix + name + suffix)
if os.path.exists(file): continue
_sys.audit("tempfile.mkdtemp", file)
_os.mkdir(file, 0o700)
return file
raise FileExistsError(_errno.EEXIST, "No usable temporary directory name found")I recognize that I do not understand the performance/stability/security implications of these changes fully, but on the surface it seems like a better approach than to rely on the try except statements from the previous code. Since the purpose of catching the try:
_os.mkdir(file, 0o700)
except FileExistsError:
continue # try again
except PermissionError:
# This exception is thrown when a directory with the chosen name
# already exists on windows, or if the process does not have proper
# permissions to create the directory.
if (_os.name == 'nt' and _os.path.exists(file)):
continue
else:
raiseAgain, i've removed the |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: