◐ Shell
reader mode source ↗
Skip to content

bpo-14094: Use _getfinalpathname to implement realpath#11248

Closed
vladima wants to merge 23 commits into
python:masterfrom
vladima:realpath
Closed

bpo-14094: Use _getfinalpathname to implement realpath#11248
vladima wants to merge 23 commits into
python:masterfrom
vladima:realpath

Conversation

@vladima

@vladima vladima commented Dec 19, 2018

Copy link
Copy Markdown
Contributor

Per discussion realpath should be using GetFinalPathNameByHandle and it is already exposed via _getfinalpathname.

https://bugs.python.org/issue14094

@zooba zooba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I can't get to bpo right now to check the discussion, but the change looks fine.

@vladima

vladima commented Dec 20, 2018

Copy link
Copy Markdown
Contributor Author

A missing bit - handling of UNC paths with \\?\ prefix - such paths will be prefixed with \\?\UNC\

@vladima

vladima commented Dec 21, 2018

Copy link
Copy Markdown
Contributor Author

I can update this PR with a suggested approach of people don't mind.

@vladima

vladima commented Jan 4, 2019

Copy link
Copy Markdown
Contributor Author

is there anything else that should be done for this PR?

@vladima

vladima commented Jan 9, 2019

Copy link
Copy Markdown
Contributor Author

@vstinner, @zooba, @eryksun - can you please tell if there is anything missing in this PR?

29 hidden items Load more…
@vladima

vladima commented Jan 14, 2019

Copy link
Copy Markdown
Contributor Author

I think all comments are addressed, is there anything else?

@eryksun

eryksun commented Jan 14, 2019

Copy link
Copy Markdown
Contributor

I think all comments are addressed, is there anything else?

Feedback from core devs would be appreciated (@vstinner, @serhiy-storchaka, @zooba, @ZackerySpytz, @pfmoore, @tjguk, @zware).

The short-name test needs to be hardened to get the short name of the directory via FindFirstFileW. Here's a function to get the short name of a file (alternatively, for the entire path, call GetShortPathNameW):

import ctypes
from ctypes import wintypes
kernel32 = ctypes.WinDLL('kernel32', use_last_error=True)

INVALID_HANDLE_VALUE = wintypes.HANDLE(-1).value
kernel32.FindFirstFileW.restype = wintypes.HANDLE
kernel32.FindClose.argtypes = (wintypes.HANDLE,)

def get_short_name(path):
    info = wintypes.WIN32_FIND_DATAW()
    h = kernel32.FindFirstFileW(path, ctypes.byref(info))
    if h == INVALID_HANDLE_VALUE:
        raise ctypes.WinError(ctypes.get_last_error())
    kernel32.FindClose(h)
    return info.cAlternateFileName

Skip the test if get_short_name returns an empty string. In this case, the file system of the temp directory may not support short names, or automatic creation of short names may be disabled.

I think more cases need to be tested, including adapting tests from posixpath, if any are relevant.

  • An input extended path should always be returned as an extended path.
  • Normal paths replace forward slash with backslash, but extended paths do not. Join the junction path with non-existent "spam/eggs". Resolve it as a normal path, and verify that the junction target is resolved and the unresolved part is normalized to "spam\eggs". Do the same but with an extended path, and verify that the unresolved part is left as "spam/eggs".
  • Normal paths resolve "." and ".." components in user mode, before making a system call. Join the junction path with ".\spam\..". Resolve it using a normal path, and verify that it's the junction target. Resolve it using an extended path, and verify that the result is an extended path for the junction target that's joined with the unresolved part ".\spam\..". (Windows diverges from Unix on this point, and it shows up especially when handling symlinks and junction mountpoints. It's also why we can lead with an abspath call in realpath, which minimizes the risk of a working-directory race-condition for a relative path and immediately resolves reserved DOS device names. File systems may reserve "." and ".." as names, but these names have no other significance in the kernel. Using an extended path, FAT32 will even let us create directories or files named "." and "..". They will then appear twice in the directory listing, except only once in the root directory.)
  • Normal paths strip trailing spaces and dots from the final component. Use an extended path to create a file named "spam. . .". Resolve it using a normal path, and verify that it returns non-existing "spam" as the name. Create a symlink to the extended-path target. The symlink privilege may be required; skip the rest if this fails. Resolve the link using a normal path, and verify that it returns an extended path for "spam. . ." . (We can't target a directory named "spam. . ." using a junction, since _winapi.CreateJunction mistakenly calls GetFullPathNameW on an extended-path target. If this gets fixed, we can use a junction instead.)
  • Normal paths reserve DOS device names in the final path component of logical-drive paths. Use an extended path to create a file named "nul: .txt"; make sure this is a logical-drive path. Resolve it using a normal path, and verify that it returns the device path "\\.\nul". Create a symlink to the extended-path target. Skip the rest if this fails. Resolve the link using a normal path, and verify that it returns an extended path for "nul: .txt".

@zooba zooba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I'd also like to see more tests. We're covering a lot of edge-cases here now, but not checking them all.

@csabella

Copy link
Copy Markdown
Contributor

@vladima, please address @zooba's last code review comments. Thanks!

@zooba

zooba commented Sep 10, 2019

Copy link
Copy Markdown
Member

This no longer applies, but thanks for all the effort you put into the change (and dealing with our feedback)!

@zooba zooba closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants