Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ntpath.realpath() does not fully resolve relative paths #82634

Closed
zooba opened this issue Oct 11, 2019 · 18 comments
Closed

ntpath.realpath() does not fully resolve relative paths #82634

zooba opened this issue Oct 11, 2019 · 18 comments
Assignees
Labels
3.8 only security fixes 3.9 only security fixes OS-windows type-bug An unexpected behavior, bug, or error

Comments

@zooba
Copy link
Member

zooba commented Oct 11, 2019

BPO 38453
Nosy @pfmoore, @vstinner, @tjguk, @zware, @eryksun, @zooba, @miss-islington
PRs
  • bpo-38453: Resolve test directories before chdir to them #16723
  • [3.8] bpo-38453: Resolve test directories before chdir to them (GH-16723) #16783
  • bpo-38453: Ensure ntpath.realpath correctly resolves relative paths #16967
  • [3.8] bpo-38453: Ensure ntpath.realpath correctly resolves relative paths (GH-16967) #17174
  • bpo-38453: Ensure correct short path is obtained for test #17184
  • [3.8] bpo-38453: Ensure correct short path is obtained for test (GH-17184) #17186
  • 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:

    assignee = 'https://github.com/zooba'
    closed_at = <Date 2019-11-18.17:32:26.461>
    created_at = <Date 2019-10-11.22:27:20.160>
    labels = ['type-bug', '3.8', '3.9', 'OS-windows']
    title = 'ntpath.realpath() does not fully resolve relative paths'
    updated_at = <Date 2019-11-18.17:32:26.446>
    user = 'https://github.com/zooba'

    bugs.python.org fields:

    activity = <Date 2019-11-18.17:32:26.446>
    actor = 'steve.dower'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2019-11-18.17:32:26.461>
    closer = 'steve.dower'
    components = ['Windows']
    creation = <Date 2019-10-11.22:27:20.160>
    creator = 'steve.dower'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38453
    keywords = ['patch']
    message_count = 18.0
    messages = ['354509', '354511', '354512', '354638', '354640', '355574', '355575', '355680', '356548', '356701', '356702', '356722', '356723', '356726', '356727', '356733', '356739', '356888']
    nosy_count = 7.0
    nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'miss-islington']
    pr_nums = ['16723', '16783', '16967', '17174', '17184', '17186']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue38453'
    versions = ['Python 3.8', 'Python 3.9']

    @zooba
    Copy link
    Member Author

    zooba commented Oct 11, 2019

    Because the abspath() call is deferred until the end of the resolution process, if the current working directory is not already a real path, it will not be fully resolved.

    By passing it through abspath() at the start of the function, we ensure that the correct path is resolved.

    This will help resolve some inconsistencies in the test suite when tests are launched with a short filename (FILENA~1) in the working directory (though some other fixes are probably needed to help here).

    @zooba zooba added 3.8 only security fixes 3.9 only security fixes OS-windows type-bug An unexpected behavior, bug, or error labels Oct 11, 2019
    @zooba
    Copy link
    Member Author

    zooba commented Oct 11, 2019

    Updating change_cwd() in Lib/test/support/init.py to call chdir(realpath(path)) also fixes the test issues I was facing, but the realpath() implementation needs fixing.

    @zooba
    Copy link
    Member Author

    zooba commented Oct 11, 2019

    PR 16723 is just for the test fix.

    The realpath() fix should get tests that chdir (without change_cwd()) into a symlink and then realpath() on a relative path. Today, that should fail to resolve the sections in the CWD, and with the fix it should resolve the entire path.

    @zooba
    Copy link
    Member Author

    zooba commented Oct 14, 2019

    New changeset d83fc27 by Steve Dower in branch 'master':
    bpo-38453: Resolve test directories before chdir to them (GH-16723)
    d83fc27

    @miss-islington
    Copy link
    Contributor

    New changeset aa909b6 by Miss Islington (bot) in branch '3.8':
    bpo-38453: Resolve test directories before chdir to them (GH-16723)
    aa909b6

    @zooba zooba changed the title ntpath.realpath() should make absolute path earlier ntpath.realpath() does not fully resolve relative paths Oct 14, 2019
    @zooba
    Copy link
    Member Author

    zooba commented Oct 28, 2019

    PR 16967 should fix the relative path resolution property, by joining with cwd if the original path is unprefixed and not absolute, and then joining with any symlink directories if their link is not absolute.

    @zooba
    Copy link
    Member Author

    zooba commented Oct 28, 2019

    I also had to special-case "realpath('nul')" to maintain the behaviour that we deliberately maintained in another bug, and I think isabs() should always return true for "\\?\" prefixed paths (given that join() will promote a "\\?\" prefixed path to the root).

    @zooba
    Copy link
    Member Author

    zooba commented Oct 29, 2019

    The discussion on the PR has raised one scenario where our algorithm conflicts with what we can reasonably emulate from the IO manager (which is itself not entirely self-consistent):

    If you call ntpath.realpath(r"C:\spam\eggs") where "spam" is a symlink/junction to "D:\foo\baz" and "D:\foo\baz\eggs" is a relative symlink to "..\bar" and "D:\foo\bar" does not exist, we will incorrectly return "C:\bar".

    Note that if "D:\foo\bar" *does* exist, we will return the correct result. And if "C:\spam" is *not* a symlink/junction, we will return the correct result.

    Essentially, we fail in the case where ntpath.exists(X) fails and realpath(dirname(X)) != dirname(X) and readlink(X).startswith("..\\") is True.

    Considering it's taken months of patient explanation from Eryk Sun for me to understand this, and nobody has ever complained about it, I'm prepared to call it a known limitation.

    @zooba
    Copy link
    Member Author

    zooba commented Nov 13, 2019

    My latest push to PR 16967 is a significant change to what ntpath.realpath will return for broken symlinks, but I think it's the right change.

    Basically, if the OS fully resolves the path, great! We'll return that (and handle \\?\ stripping)

    If the OS doesn't, we'll assume it's a broken link and try and read the link target from the path. Most of the time, we'll get "not a link", but if it succeeds then we'll return here. For absolute links, we'll return it. For relative symlinks, we'll join with the parent directory. For relative anything-else, we'll return the path to the link itself.

    If we can't read a link (most likely), then we'll work up the path doing the same thing. Once we successfully get further than the original path, we'll join the unprocessed tail on and return without attempting to resolve anything more (assuming that the OS would have done it earlier if it was possible).

    So if you look at the changed test_ntpath.py you'll see the different results (the test_shutil.py changes are mainly to avoid long/short name comparison failures when realpath() fixes them, by putting the test files in the actual test directory instead of global TEMP).

    @zooba
    Copy link
    Member Author

    zooba commented Nov 15, 2019

    New changeset abde52c by Steve Dower in branch 'master':
    bpo-38453: Ensure ntpath.realpath correctly resolves relative paths (GH-16967)
    abde52c

    @zooba
    Copy link
    Member Author

    zooba commented Nov 15, 2019

    I think the PR as it stands is a net improvement over where we currently are, so I'll merge it. We can make more tweaks as necessary.

    @vstinner
    Copy link
    Member

    AMD64 Windows8.1 Non-Debug 3.x is grumpy:
    https://buildbot.python.org/all/#/builders/12/builds/3459

    ======================================================================
    FAIL: test_realpath_cwd (test.test_ntpath.TestNtpath)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "D:\buildarea\3.x.ware-win81-release.nondebug\build\lib\test\test_ntpath.py", line 424, in test_realpath_cwd
        self.assertPathEqual(test_file_long, ntpath.realpath(test_file_short))
      File "D:\buildarea\3.x.ware-win81-release.nondebug\build\lib\test\test_ntpath.py", line 62, in assertPathEqual
        self.assertEqual(path1, path2)
    AssertionError: 'D:\\[88 chars]worker_4476\\@test_4476_tmp\\MyVeryLongDirectoryName\\file.txt' != 'D:\\[88 chars]worker_4476\\@test_4476_tmp\\MYVERY~1\\file.txt'
    - D:\buildarea\3.x.ware-win81-release.nondebug\build\build\test_python_4088\test_python_worker_4476\@test_4476_tmp\MyVeryLongDirectoryName\file.txt
    ?                                                                                                                   ^ ^^^^^^^^^^^^^^^^^^^^
    + D:\buildarea\3.x.ware-win81-release.nondebug\build\build\test_python_4088\test_python_worker_4476\@test_4476_tmp\MYVERY~1\file.txt
    ?                                                                                                                   ^ ^^^^^

    @vstinner
    Copy link
    Member

    Grumpy AMD64 Windows10 3.x:
    https://buildbot.python.org/all/#/builders/3/builds/3788

    ======================================================================
    FAIL: test_realpath_cwd (test.test_ntpath.TestNtpath)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "D:\buildarea\3.x.bolen-windows10\build\lib\test\test_ntpath.py", line 424, in test_realpath_cwd
        self.assertPathEqual(test_file_long, ntpath.realpath(test_file_short))
      File "D:\buildarea\3.x.bolen-windows10\build\lib\test\test_ntpath.py", line 62, in assertPathEqual
        self.assertEqual(path1, path2)
    AssertionError: 'D:\\[76 chars]worker_6956\\@test_6956_tmp\\MyVeryLongDirectoryName\\file.txt' != 'D:\\[76 chars]worker_6956\\@test_6956_tmp\\MYVERY~1\\file.txt'
    - D:\buildarea\3.x.bolen-windows10\build\build\test_python_6236\test_python_worker_6956\@test_6956_tmp\MyVeryLongDirectoryName\file.txt
    ?                                                                                                       ^ ^^^^^^^^^^^^^^^^^^^^
    + D:\buildarea\3.x.bolen-windows10\build\build\test_python_6236\test_python_worker_6956\@test_6956_tmp\MYVERY~1\file.txt
    ?                                                                                                       ^ ^^^^^

    pythoninfo:

    windows.RtlAreLongPathsEnabled: False

    @zooba
    Copy link
    Member Author

    zooba commented Nov 15, 2019

    New changeset 66c0f01 by Steve Dower in branch '3.8':
    bpo-38453: Ensure ntpath.realpath correctly resolves relative paths (GH-16967)
    66c0f01

    @zooba
    Copy link
    Member Author

    zooba commented Nov 15, 2019

    Thanks, Victor! That looks different from the one I was originally fixing (even though the long/short path mismatch is there), so I'll take a look.

    @zooba
    Copy link
    Member Author

    zooba commented Nov 16, 2019

    New changeset 7c6130c by Steve Dower in branch 'master':
    bpo-38453: Ensure correct short path is obtained for test (GH-17184)
    7c6130c

    @miss-islington
    Copy link
    Contributor

    New changeset 6f602fb by Miss Islington (bot) in branch '3.8':
    bpo-38453: Ensure correct short path is obtained for test (GH-17184)
    6f602fb

    @zooba
    Copy link
    Member Author

    zooba commented Nov 18, 2019

    Closing this as I believe it's done, but happy to reopen if we find another edge case (or start a new issue).

    @zooba zooba closed this as completed Nov 18, 2019
    @zooba zooba self-assigned this Nov 18, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants