Skip to content

gh-96290: support partial/invalid UNC drives in normpath() and splitdrive()#100351

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.

Sign up for GitHub

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

Jump to bottom
Merged
zooba merged 11 commits into from
Jan 12, 2023
Merged

gh-96290: support partial/invalid UNC drives in normpath() and splitdrive() #100351

zooba merged 11 commits into from
Jan 12, 2023
+55 −46

Conversation

@barneygale
Copy link
Contributor

@barneygalebarneygale commented Dec 19, 2022•
edited
Loading

Fix handling of partial and invalid UNC drives in ntpath.splitdrive(), and in ntpath.normpath() on non-Windows systems.

Paths such as \\server and \\ are now considered by splitdrive() to contain only a drive, and consequently are not modified by normpath() on non-Windows systems. The behaviour of normpath() on Windows systems is unaffected, as native OS APIs are used.

This brings the Python implementation of ntpath.normpath() more in line with the C implementation added in 99fcf15.

Implementation by @eryksun. I added tests and NEWS.

@bedevere-botbedevere-bot mentioned this pull request Dec 19, 2022
Closed
@barneygalebarneygaleforce-pushed the gh-96290-ntpath-normpath-fix branch from 471c47b to 7ab642bCompareDecember 20, 2022 00:13
@barneygale@eryksun
pythongh-96290: support partial/invalid UNC drives in normpath() and …
1fb70c7
…splitdrive() This brings the Python implementation of `ntpath.normpath()` in line with the C implementation added in 99fcf15 Co-authored-by: Eryk Sun <eryksun@gmail.com>
@barneygalebarneygaleforce-pushed the gh-96290-ntpath-normpath-fix branch from 7ab642b to 1fb70c7CompareDecember 20, 2022 00:54
@barneygalebarneygale marked this pull request as ready for review December 20, 2022 00:55
@brettcannonbrettcannon requested review from a team and eryksunDecember 20, 2022 01:15
@barneygalebarneygale mentioned this pull request Dec 20, 2022
Merged
@eryksun
Copy link
Contributor

eryksun commented Dec 22, 2022

This brings the Python implementation of ntpath.normpath() in line with the C implementation

Almost. The C implementation is incorrect for "UNC" device paths. For example:

>>> nt._path_normpath('//?/UNC/server/share/..') '\\\\?\\UNC\\server'

The problem is that it handles "//?/UNC/" as the root instead of "//?/UNC/server/share/". The pure Python implementation gets it right.

@barneygale
Copy link
ContributorAuthor

barneygale commented Dec 22, 2022

I've changed "in line" to "more in line" :-)

eryksun
eryksun reviewed Dec 27, 2022
View reviewed changes
eryksun
eryksun reviewed Dec 27, 2022
View reviewed changes
eryksun
eryksun reviewed Dec 27, 2022
View reviewed changes
barneygaleand others added 2 commits December 28, 2022 22:56
@barneygale@eryksun
Update Lib/ntpath.py
d2818a3
Co-authored-by: Eryk Sun <eryksun@gmail.com>
@barneygale
Fix tests
5965f0f
@barneygalebarneygale requested review from eryksun and removed request for brettcannonDecember 29, 2022 15:34
eryksun
eryksun reviewed Dec 29, 2022
View reviewed changes
@barneygale@eryksun
Update Lib/test/test_ntpath.py
58d5d9f
Co-authored-by: Eryk Sun <eryksun@gmail.com>
eryksun
eryksun reviewed Dec 29, 2022
View reviewed changes
@barneygale@eryksun
Update Lib/test/test_ntpath.py
bad4b5d
Co-authored-by: Eryk Sun <eryksun@gmail.com>
eryksun
eryksun reviewed Dec 29, 2022
View reviewed changes
@barneygale@eryksun
Update Lib/test/test_ntpath.py
5b67a74
Co-authored-by: Eryk Sun <eryksun@gmail.com>
zooba
zooba reviewed Jan 10, 2023
View reviewed changes
@barneygalebarneygale added 3.11 only security fixes 3.12 only security fixes needs backport to 3.11 only security fixes and removed 3.11 only security fixes 3.12 only security fixes labels Jan 11, 2023
@zooba
Copy link
Member

zooba commented Jan 11, 2023

If Eryk and Barney both say this is ready, I'm happy to merge.

@barneygale
Copy link
ContributorAuthor

barneygale commented Jan 12, 2023

It's ready to land I think!

@eryksun
Copy link
Contributor

eryksun commented Jan 12, 2023•
edited
Loading

A long-standing issue with ntpath.isabs() is fixed, and it no longer depends on splitdrive(). We need a couple of tests in test_isabs(). I'd like to test a normal UNC path and a device path, both without a trailing backslash. They're really the same case as far as the new implementation is concerned, but I feel better with both being tested. In the old implementation, isabs() incorrectly returns false for these two cases.

tester('ntpath.isabs("\\\\conky\\mountpoint")', 1) tester('ntpath.isabs("\\\\.\\C:")', 1)

@eryksun
Copy link
Contributor

eryksun commented Jan 12, 2023

@zooba, it's ready to be merged.

@zoobazooba merged commit 005e694 into python:mainJan 12, 2023
@miss-islington
Copy link
Contributor

miss-islington commented Jan 12, 2023

Thanks @barneygale for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Jan 12, 2023

Sorry, @barneygale and @zooba, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 005e69403d638f9ff8f71e59960c600016e101a4 3.11

@bedevere-bot
Copy link

bedevere-bot commented Jan 12, 2023

GH-100999 is a backport of this pull request to the 3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11 only security fixes label Jan 12, 2023
zooba pushed a commit to zooba/cpython that referenced this pull request Jan 12, 2023
@barneygale@eryksun@zooba
pythongh-96290: Support partial/invalid UNC drives in ntpath.normpath…
77d4875
…() and splitdrive() (pythonGH-100351) This brings the Python implementation of `ntpath.normpath()` in line with the C implementation added in 99fcf15 Co-authored-by: Eryk Sun <eryksun@gmail.com>
zooba added a commit that referenced this pull request Jan 12, 2023
@zooba@barneygale@eryksun
gh-96290: Support partial/invalid UNC drives in ntpath.normpath() and…
55a26de
… splitdrive() (GH-100351) This brings the Python implementation of `ntpath.normpath()` in line with the C implementation added in 99fcf15 Co-authored-by: Barney Gale <barney.gale@gmail.com> Co-authored-by: Eryk Sun <eryksun@gmail.com>
@barneygale
Copy link
ContributorAuthor

barneygale commented Jan 12, 2023

Thank you both!

@rmartin16rmartin16 mentioned this pull request Feb 8, 2023
Closed
@Grub4KGrub4K mentioned this pull request Sep 17, 2023
Closed
10 tasks
@barneygalebarneygale mentioned this pull request Jan 4, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zoobazoobazooba left review comments

@eryksuneryksunAwaiting requested review from eryksun

Assignees

@zoobazooba

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@barneygale@eryksun@zooba@miss-islington@bedevere-bot