classification
Title: os.link(..., follow_symlinks=False) without linkat(3)
Type: behavior Stage: patch review
Components: Extension Modules Versions: Python 3.10
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eryksun, gregory.p.smith, pablogsal, ronaldoussoren
Priority: normal Keywords: patch

Created on 2020-07-21 09:28 by ronaldoussoren, last changed 2020-07-29 21:14 by gregory.p.smith.

Pull Requests
URL Status Linked Edit
PR 21648 closed pablogsal, 2020-07-27 23:25
Messages (7)
msg374057 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-07-21 09:28
The code for os.link() seems to ignore follow_symlinks when the linkat(2) function is not available on the platform, which results in unexpected behaviour when "follow_symlinks" is false.
msg374459 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2020-07-28 03:37
I'm trying to give os.link() and follow_symlinks the benefit of the doubt, but the implementation just seems buggy to me. 

POSIX says that "[i]f path1 names a symbolic link, it is implementation-defined whether link() follows the symbolic link, or creates a new link to the symbolic link itself" [1]. In Linux, link() does not follow symlinks. One has to call linkat() with AT_SYMLINK_FOLLOW:

    AT_SYMLINK_FOLLOW (since Linux 2.6.18)
        By default, linkat(), does not dereference oldpath if it is a 
        symbolic link (like link()). The flag AT_SYMLINK_FOLLOW can be
        specified in flags to cause oldpath to be dereferenced if it is
        a symbolic link. 

The behavior is apparently the same in FreeBSD [2]. 

Thus the following implementation in os.link() seems buggy.

#ifdef HAVE_LINKAT
    if ((src_dir_fd != DEFAULT_DIR_FD) ||
        (dst_dir_fd != DEFAULT_DIR_FD) ||
        (!follow_symlinks))
        result = linkat(src_dir_fd, src->narrow,
            dst_dir_fd, dst->narrow,
            follow_symlinks ? AT_SYMLINK_FOLLOW : 0);
    else
#endif /* HAVE_LINKAT */

The only way that the value of follow_symlinks matters in Linux is if src_dir_fd or dst_dir_fd is used with a real file descriptor (i.e. not DEFAULT_DIR_FD, which is AT_FDCWD). Otherwise, the default True value of follow_symlinks is an outright lie. For example:

    >>> os.link in os.supports_follow_symlinks
    True
    >>> open('spam', 'w').close()
    >>> os.symlink('spam', 'spamlink1')
    >>> os.link('spamlink1', 'spamlink2')

spamlink2 was created as a hardlink to spamlink1, not its target, i.e. it's a symlink:
 
    >>> os.lstat('spamlink1').st_ino == os.lstat('spamlink2').st_ino
    True
    >>> os.readlink('spamlink2')
    'spam'

In contrast, if src_dir_fd is passed, then follow_symlinks=True is implemented as advertised (via AT_SYMLINK_FOLLOW):

    >>> fd = os.open('.', 0)
    >>> os.link('spamlink1', 'spamlink3', src_dir_fd=fd)

spamlink3 was created as a hardlink to spam, the target of spamlink1:
  
    >>> os.lstat('spam').st_ino == os.lstat('spamlink3').st_ino
    True

That the value of an unrelated parameter -- src_dir_fd -- changes the behavior of the follow_symlinks parameter is obviously a bug that should be addressed.

POSIX mandates that "[i]f both fd1 and fd2 have value AT_FDCWD, the behavior shall be identical to a call to link(), except that symbolic links shall be handled as specified by the value of flag". It's already using AT_FDCWD as a default value, so the implementation of os.link() should just unconditionally call linkat() if it's available. Then the value of follow_symlinks, true or false, will be honored, with or without passing src_dir_fd or dst_dir_fd.

That said, since os.link() hasn't been working as advertised, this change needs to be accompanied by changing the default value of follow_symlinks to False. That will retain the status quo behavior for most systems, except in the rare case that src_dir_fd or dst_dir_fd is used. If it isn't changed to False, then suddenly os.link() calls will start following symlinks, whereas prior to the change they did not because link() was being called instead of linkat(). 

--- 

In Windows, CreateHardLinkW [3] is incorrectly documented as following symlinks (i.e. "[i]f the path points to a symbolic link, the function creates a hard link to the target"). Actually, it opens the file to be hard-linked with the NTAPI option FILE_OPEN_REPARSE_POINT (same as WinAPI FILE_FLAG_OPEN_REPARSE_POINT). Thus no type of reparse point is followed, including symlinks.

---

[1]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/link.html
[2]: https://www.unix.com/man-page/FreeBSD/2/link
[3]: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createhardlinkw
msg374485 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-07-28 11:06
> That said, since os.link() hasn't been working as advertised, this change needs to be accompanied by changing the default value of follow_symlinks to False. That will retain the status quo behavior for most systems, except in the rare case that src_dir_fd or dst_dir_fd is used. If it isn't changed to False, then suddenly os.link() calls will start following symlinks, whereas prior to the change they did not because link() was being called instead of linkat(). 


Isn't that a backwards-incompatible change?
msg374488 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2020-07-28 11:48
> Isn't that a backwards-incompatible change?

So, do you think it should just be documented that follow_symlinks is effectively ignored with os.link() on most platforms that claim to support it, unless either src_dir_fd or dst_dir_fd is used? I'd rather it was fixed to behave consistently in 3.10, even if it's backwards incompatible with some use cases on some platforms. I think for most use cases, it's just called without arguments as os.link(src, dst), in which case on most platforms switching the default to follow_symlinks=False will preserve the existing and expected behavior.
msg374500 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-07-28 15:00
I agree that the current implementation is wonky.  

The implementation should use linkat(2) whenever it is available, that's the only portable way to honour the follow_symlinks flag as POSIX says that the behaviour for link(2) with symbolic links is implementation defined.

From a quick experiment link(2) on Linux behaves like linkat(2) without AT_SYMLINK_FOLLOW.  On macOS link(2) behaves like linkat(2) *with* AT_SYMLINK_FOLLOW.   

That means os.link behaviour is currently different on macOS and Linux. 

I think it would be worthwhile to try to standardise the behaviour. Given the relative market sizes it I'd go for the current behaviour on Linux (with explicit tests!), even if that might not be backward compatible on macOS.

I'd also add a configure test for the behaviour of link(2) and error out when the user specifies a value for follow_symlinks that's not compatible with link(2) when linkat(2) is not available.  Or maybe only do this when the user explicitly passes in a value for this argument (make it a tri-state).

Also: note that the current macOS installers on macOS don't look at the follow_symlinks flag at all, they are build on macOS 10.9 where linkat(2) is not available (unlink macOS 10.10 or later).  That's why I noticed this problem.
msg374513 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-07-28 17:45
> So, do you think it should just be documented that follow_symlinks is effectively ignored with os.link() on most platforms that claim to support it, unless either src_dir_fd or dst_dir_fd is used?

At this stage, I am just trying to understand all the possibilities in the design space and I don't have a preferred path. I just wanted to point out that we should take into account that many things may be broken if we make changes that are backwards-incompatible in a low-level function and we must have that in mind
msg374586 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2020-07-29 21:14
Thanks for the analysis Eryk!  I think you are right, changing the default to match the behavior that people have actually been experiencing on `os.link(src, dst)` makes sense.

Possible suggestion:

We can go one step further if anyone believes it is likely to be necessary: Preserve the exiting buggy behavior regardless of src_dir_fd= value when follow_symlinks is not explicitly provided as an argument.  This way the behavior for people who did not specify it remains unchanged <= 3.9.  This would be the principal of no surprise.  (it'd effectively become a tri-state _UNSPECIFIED/True/False value where _UNSPECIFIED depends on the mess of conditionals described by Eryk)

Documentation up through 3.9 should be updated to note the oddity.

In 3.8 & 3.9 if it _is_ explicitly specified, fixing the bug so that it actually honors that makes sense.

In 3.10 we should honor the new =False default without this tri-state behavior bug-compatible-by-default at all.

This is more complicated to implement.  I'd also be happy with the already described "just updating the default to False and fixing forward in 3.10 to actually honor True properly."

META: Regarding macOS, can we update the macOS version used to build the installers for 3.10?
History
Date User Action Args
2020-07-29 21:14:58gregory.p.smithsetmessages: + msg374586
2020-07-28 17:46:11pablogsalsetnosy: + gregory.p.smith
2020-07-28 17:45:22pablogsalsetmessages: + msg374513
2020-07-28 15:00:45ronaldoussorensetmessages: + msg374500
2020-07-28 11:48:26eryksunsetmessages: + msg374488
2020-07-28 11:06:41pablogsalsetmessages: + msg374485
2020-07-28 03:37:21eryksunsetnosy: + eryksun
messages: + msg374459
2020-07-27 23:25:18pablogsalsetkeywords: + patch
nosy: + pablogsal

pull_requests: + pull_request20787
stage: test needed -> patch review
2020-07-21 09:28:04ronaldoussorencreate