bpo-43757: make pathlib use os.path.realpath() to resolve all symlinks in a path

[barneygale]

Removes a pathlib-specific implementation of realpath() that's mostly
copied from ntpath and posixpath.

https://bugs.python.org/issue43757

[barneygale]

Summary of the differences between os.path.realpath() and pathlib.Path.resolve().

Before this patch:

	symlink loop	missing ancestors
os.path.realpath(p)	return path	muddle on!
p.resolve(strict=False)	raise RuntimeError	muddle on!
p.resolve(strict=True)	raise RuntimeError	raise OSError

After this patch:

	symlink loop	missing ancestors
os.path.realpath(p, strict=False)	raise OSError	muddle on!
os.path.realpath(p, strict=True)	raise OSError	raise OSError
p.resolve(strict=False)	raise RuntimeError	muddle on!
p.resolve(strict=True)	raise RuntimeError	raise OSError

[eryksun]

Note that ntpath._getfinalpathname_nonstrict() currently ignores ERROR_CANT_RESOLVE_FILENAME (1921) in order to match the behavior of posixpath.realpath() for a symlink loop. It has to be removed from the list of allowed errors if you want strict=False to raise OSError for a reparse loop.

[barneygale]

Note that ntpath._getfinalpathname_nonstrict() currently ignores ERROR_CANT_RESOLVE_FILENAME (1921) in order to match the behavior of posixpath.realpath() for a symlink loop. It has to be removed from the list of allowed errors if you want strict=False to raise OSError for a reparse loop.

Thanks. I've made this adjustment and also rebased to fix conflicts.

[barneygale]

Looks like the realpath() symlink loop behaviour dates back to bpo-930024:

Create a symlink pointing to itself: ln -s infinite infinite
/home/amk/infinite is a perfectly good path, though it can't be followed.

This doesn't agree with my expectations, nor with the first sentence of the function documentation:

Return the canonical path of the specified filename, eliminating any symbolic links encountered in the path

A symlink loop doesn't seem like a good reason to break the bolded guarantee.

[barneygale]

Path.resolve() currently raises a RuntimeError when it hits a symlink loop. Arguably an OSError would make more sense, and would conform with the proposed realpath() behaviour. Any thoughts on whether it's worth changing?

[eryksun]

Path.resolve() currently raises a RuntimeError when it hits a symlink loop. Arguably an OSError would make more sense, and would conform with the proposed realpath() behaviour. Any thoughts on whether it's worth changing?

I have nothing useful to add. I think @serhiy-storchaka is the person to ask.

[zooba]

Not declaring an opinion yet on the behaviour change, but I think it's probably okay. I'd be happier if the link cycle change was also tied to the strict argument. (My main concern is that virtually nobody will be testing for it, and it can become an issue based entirely on user's setup - nothing to do with the code that will fail.)

[barneygale]

Not declaring an opinion yet on the behaviour change, but I think it's probably okay. I'd be happier if the link cycle change was also tied to the strict argument. (My main concern is that virtually nobody will be testing for it, and it can become an issue based entirely on user's setup - nothing to do with the code that will fail.)

I can make the new symlink loop handling optional, but in order to use it from pathlib it would need to be an additional argument, as pathlib.Path.resolve() always raises on broken symlinks regardless of the value of strict. So I'd need to adjust this patch to add two arguments to realpath(), which I'm happy to do if we feel it's the way forward.

I'd argue that the new behaviour is justified because the current behaviour seems entirely non-useful. I can't find another example of a realpath function/executable that doesn't error out when it hits a symlink loop - even PHP's version seems to return false in this case. I suspect the system misconfiguration in your example will result in an exception regardless of this change, as the result of realpath() will be fed into something that will barf on the symlink loop. Better to fail earlier?

[zooba]

I suspect the system misconfiguration in your example will result in an exception regardless of this change, as the result of realpath() will be fed into something that will barf on the symlink loop. Better to fail earlier?

This is probably true, but I don't want to add an earlier (and less expected) failure without more warning. Raising a warning in this case but not an error would be fine for 3.10 and 3.11, and then we could confidently make it raise an error in 3.12. That gives devs a few years to realise that an error might be occurring here and handle it properly (or choose to suppress it explicitly).

[barneygale]

Sounds very reasonable, thanks! I'll revise this patch.

[barneygale]

Some options for how we could retain the current realpath() behaviour and still remove the duplicate implementation in pathlib:

1. We could make Path.resolve() match realpath() behaviour, i.e. don't raise for symlink loops in non-strict mode.
   • Just moves the behaviour change elsewhere
   • Seems like a step backwards
2. We could add another argument to realpath() that controls whether to raise on symlink loops. strict_loops? raise_on_symlink_loop?
   • Makes the realpath() signature a bit unwieldy
   • Hard to document a reasonable use case for strict_loops=False
3. We could perform some post-processing of the return value of realpath() in Path.resolve(). Basically lstat() every ancestor, and if we detect a symlink, raise an error.
   • Entails making additional lstat() calls, which could be slower

Leaning towards option 3. Any thoughts, or options I've missed?

[barneygale]

OK, I've updated this such that there's no change in non-strict realpath() behaviour (the default), so this should be fully backwards-compatible.

Before this PR:

	symlink loop	other OSError
os.path.realpath(p)	muddle on	muddle on
p.resolve()	raise RuntimeError	muddle on
p.resolve(strict=True)	raise RuntimeError	raise OSError

After this PR:

	symlink loop	other OSError
os.path.realpath(p)	muddle on	muddle on
os.path.realpath(p, strict=True)	raise OSError	raise OSError
p.resolve()	raise RuntimeError	muddle on
p.resolve(strict=True)	raise RuntimeError	raise OSError

(by "muddle on" I mean append the remaining path components and return)

[zooba]

I'm happy with this now, just want to wait for one more approval before merging.

[zooba]

Going to interpret silence as approval. Good work! Thanks for this patch.

[barneygale]

Thanks very much for your help!

[bedevere-app]

GH-135035 is a backport of this pull request to the 3.9 branch.