bpo-31226: Distinguish win junction links against volume mount points #5998
Conversation
- Adds a function _Py_is_reparse_link on windows that checks if a specific reparse point is a link or not. Returns true for symlinks, or junction links (i.e. junctions that are not volume mount points). - Uses this function instead of comparing against `IO_REPARSE_TAG_SYMLINK` where appropriate.
Modules/posixmodule.c
Outdated
| { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "not a symbolic link"); | ||
| return NULL; | ||
| } | ||
| print_name = (wchar_t *)((char*)rdb->SymbolicLinkReparseBuffer.PathBuffer + | ||
| rdb->SymbolicLinkReparseBuffer.PrintNameOffset); | ||
| if (rdb->ReparseTag == IO_REPARSE_TAG_SYMLINK) |
There was a problem hiding this comment.
If we were to generalize _Py_is_reparse_link to use the IsReparseTagNameSurrogate macro to check for a link, then IsReparseTagMicrosoft would need to be checked here. If it's a Microsoft name-surrogate tag, then we would continue with the rdb REPARSE_DATA_BUFFER pointer. If it's not a junction or symlink, we could return a tuple of "Microsoft" and the generic data buffer as bytes. If it's a non-Microsoft name-surrogate (are there any?), then we would switch to an rgdb REPARSE_GUID_DATA_BUFFER pointer and return a tuple of the GUID as a "{...}" string and the generic data as bytes.
Would that be useful to anyone? I don't know. But it would allow supporting more than just Microsoft junction and symlink reparse points as 'links' in os.stat, while remaining internally consistent.
There was a problem hiding this comment.
Changing the return type of readlink seems overly aggressive for this PR, as it was previously just string/None (thinking about backwards compatibility). The changes should be rather easy to implement in a subsequent PR though.
|
I think I've addressed all the review comments, and tests are now passing. I would appreciate any help in writing good tests though, especially for volume mount points. |
| @@ -1652,56 +1589,59 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result, | |||
| If the latter, we can use attributes_from_dir. */ | |||
| DWORD lastError = GetLastError(); | |||
| if (lastError != ERROR_ACCESS_DENIED && | |||
| lastError != ERROR_SHARING_VIOLATION) | |||
| lastError != ERROR_SHARING_VIOLATION && | |||
| lastError != ERROR_INVALID_PARAMETER) | |||
| return -1; | |||
| /* Could not get attributes on open file. Fall back to | |||
| reading the directory. */ | |||
| if (!attributes_from_dir(path, &info, &reparse_tag)) | |||
| /* Very strange. This should not fail now */ | |||
There was a problem hiding this comment.
This comment itself is very strange. Of course it can fail if you don't have access to the directory, e.g. it's in another user's profile and you're not an administrator.
|
Vidar, PR needs to be updated. |
|
I merged master and resolved the conflicts (correctly I hope, but worth a check). I'm seeing some test failures locally, but the same tests fail on master. I've included the pattern below for completeness: Local test failures follow this pattern:FAIL: test_basic (test.test_tempfile.TestMkdtemp)
----------------------------------------------------------------------
Traceback (most recent call last):
File "<dev>\cpython\lib\test\test_tempfile.py", line 701, in test_basic
os.rmdir(self.do_create())
File "<dev>\cpython\lib\test\test_tempfile.py", line 693, in do_create
self.nameCheck(name, dir, pre, suf)
File "<dev>\cpython\lib\test\test_tempfile.py", line 100, in nameCheck
"file %r not in directory %r" % (name, dir))
AssertionError: 'C:\\Users\\<username>\\AppData\\Local\\Temp' != 'C:\\Users\\<username>\\AppData\\Local\\Temp\\'
- C:\Users\<username>\AppData\Local\Temp
+ C:\Users\<username>\AppData\Local\Temp\
? +
: file 'C:\\Users\\<username>\\AppData\\Local\\Temp\\jhd6dxxm' not in directory 'C:\\Users\\<username>\\AppData\\Local\\Temp\\'
|
|
Let me know if this looks good for merging, and I'll resolve the fresh conflicts! |
Co-Authored-By: Steve Dower <steve.dower@microsoft.com>
|
Thanks for the reviews @ZackerySpytz and @zooba . I think I should have addressed your points now. |
|
The recent flurry of activity brought this issue to my attention again, and I'm thinking over whether this is the right way to handle the problem. It might be a category error to group junctions in with symbolic links. Regardless of the target, they always act like mountpoints, either as volume mountpoints or something akin to Unix bind mounts (e.g. with respect to how relative symlinks traverse them as namespace grafts). I'll post a message on the bpo issue in a day or two. |
|
Closing this as we've resolved it elsewhere. Thanks for the contribution! |
_Py_is_reparse_linkon windows that checks if a specific reparse point is a link or not. Returns true for symlinks, and for junctions that are not volume mount points.IO_REPARSE_TAG_SYMLINKwhere appropriate.https://bugs.python.org/issue31226