Ensure ntpath.realpath() result is a final, normalized pathname.

@eryksun

Bug report

Bug description:

Note that _readlink_deep() does nothing to ensure that the result is a final, normalized pathname. It just tries to keep reading symlinks until the target isn't a symlink or access/reading fails in a manner that's allowed. If it succeeds, the resulting pathname may include any number of symlinks, mount points, and short names. _getfinalpathname_nonstrict() should instead pass both the pathname and the seen set into _readlink_deep(). If it succeeds, then add the resulting pathname to the set, and continue the loop to try to get a final, normalized pathname.

Originally posted by @eryksun in #110298 (comment)

I was able to reproduce this issue. Run the following code block with Administrator privileges in Windows:

mkdir C:\testdir
mkdir C:\testdir\real
echo 1 > C:\testdir\real\file.txt
icacls C:\testdir\real\file.txt /deny *S-1-5-32-545:(S)
mklink /d C:\testdir\symlink1 C:\testdir\real
mklink /d C:\testdir\symlink2 C:\testdir\symlink1
mklink C:\testdir\symlink2\filelink.txt C:\testdir\symlink1\file.txt
python -c "import ntpath; print(ntpath.realpath('C:\\testdir\\symlink2\\filelink.txt'))"

REM Cleanup
icacls C:\testdir\real\file.txt /grant *S-1-5-32-545:(S)
rmdir C:\testdir /s /q

Expected output:

C:\testdir\real\file.txt

Actual output:

C:\testdir\symlink1\file.txt

This happens because _getfinalpathname_nonstrict() returns the result of _readlink_deep() if the return value is different from the input.

cpython/Lib/ntpath.py

Lines 680 to 686 in 92ca90b

    
           try: 
        
               # The OS could not resolve this path fully, so we attempt 
        
               # to follow the link ourselves. If we succeed, join the tail 
        
               # and return. 
        
               new_path = _readlink_deep(path) 
        
               if new_path != path: 
        
                   return join(new_path, tail) if tail else new_path

However, because _readlink_deep() does not ensure that it's result is a final, normalized pathname, ntpath.realpath() result is not ensured to be a final, normalized pathname.

I'll try to come up with a patch. @eryksun, if you have any further discussions or comments on how this should be fixed, please let me know in the comments.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Windows

Linked PRs

gh-110495: ensure ntpath.realpath() result is a final, normalized pathname. #110751

@barneygale

CC @barneygale

It seems like it happens because we don't try to _getfinalpathname on each element of the path? _readlink_deep is only attempting to resolve a link, but the result you're getting here contains a link, but is not actually a link.

Personally, I'm inclined to not try too hard when the final file isn't accessible. If there's a way we can easily tell that some segment of the path is actually a link without having to test every single one, then maybe it's worth it. But if someone has constructed a symlink chain as complex as this and made the final file inaccessible, I'm okay with letting them win.

This should be a simple fix. Just pass the seen set to _readlink_deep as a parameter. If it succeeds, _getfinalpathname_nonstrict adds the result to the set and continues the loop, resolving the path down to the device/share.

You mean the fix is to not return, but to replace path, update tail and keep looping until a _getfinalpathname call succeeds? Then we also need a second seen set for cycle detection, but I think we need to store full paths in it and not the partial (parent directory) we're working with in subsequent iterations inside nonstrict.

Maybe it won't matter, because if a partial path happens to collide with a full path we've already seen then we've still got a cycle? It's too late at night for me to puzzle that one out in my head.

Either way, I'm not convinced this is a simple fix. It might be a small code change, but we need to explain the logic as well, if not in code, then in one of those massive comments that ends up being longer than the entire function (and we're already pretty close to that with this function...)

Only a single seen set is needed. It gets passed into _readlink_deep as a parameter. For _readlink_deep, other than changing seen to be a parameter, nothing else needs to change. I neglected to mention that if _readlink_deep returns a path that's already in seen, _getfinalpathname_nonstrict() should break out of its loop. Otherwise, add the result to the set and continue the loop. The seen set wouldn't be used anywhere else in _getfinalpathname_nonstrict(). It's just meant to ensure that calling _readlink_deep returns if it encounters any path that was seen by either a previous _readlink_deep call or the current call.

Merge branch 'main' into pythongh-110495-ensure-ntpath-realpath-final…

Navigation Menu

Repository navigation

Ensure `ntpath.realpath()` result is a final, normalized pathname. #110495

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	try:
	# The OS could not resolve this path fully, so we attempt
	# to follow the link ourselves. If we succeed, join the tail
	# and return.
	new_path = _readlink_deep(path)
	if new_path != path:
	return join(new_path, tail) if tail else new_path

Ensure ntpath.realpath() result is a final, normalized pathname. #110495

Description

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Activity

sobolevn commented on Oct 7, 2023

zooba commented on Oct 9, 2023

eryksun commented on Oct 9, 2023

zooba commented on Oct 9, 2023

eryksun commented on Oct 10, 2023

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Ensure `ntpath.realpath()` result is a final, normalized pathname. #110495