Add failover for follow_symlinks and effective_ids where possible

[larryhastings]

BPO	15159
Nosy	@birkenfeld, @pitrou, @larryhastings, @hynek, @serhiy-storchaka
Files	larry.kinder.gentler.follow_symlinks.1.diff: Patch Support "bpo-" in Misc/NEWS #1 implementing graceful failover for follow_symlinks and effective_ids.

^{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/larryhastings'
closed_at = <Date 2012-06-24.11:02:57.210>
created_at = <Date 2012-06-24.05:06:12.049>
labels = ['type-bug']
title = 'Add failover for follow_symlinks and effective_ids where possible'
updated_at = <Date 2012-06-24.11:02:57.209>
user = 'https://github.com/larryhastings'

bugs.python.org fields:

activity = <Date 2012-06-24.11:02:57.209>
actor = 'larry'
assignee = 'larry'
closed = True
closed_date = <Date 2012-06-24.11:02:57.210>
closer = 'larry'
components = []
creation = <Date 2012-06-24.05:06:12.049>
creator = 'larry'
dependencies = []
files = ['26122']
hgrepos = []
issue_num = 15159
keywords = ['patch']
message_count = 4.0
messages = ['163713', '163750', '163761', '163767']
nosy_count = 6.0
nosy_names = ['georg.brandl', 'pitrou', 'larry', 'neologix', 'hynek', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue15159'
versions = []

[larryhastings]

Serhiy Storchaka suggested (in private email, not on tracker or python-dev): why not make follow_symlinks and effective_ids failover where possible?

Let's take the example of effective_ids first, that's simpler. Let's say the user calls
os.access("x", os.F_OK, effective_ids=True)
But they doesn't have faccessat() for some reason. IF euid==uid, and egid==gid, then it's harmless to ignore the effective_ids flag and just use normal access().

Supporting this is easy: if effective_ids=True, and !defined(HAVE_FACCESSAT), but we have all four of the get{e|}{u|g}id() functions, do the above comparison and if it is just call access().

It's a bit more complicated with follow_symlinks. Let's say they call
os.chmod("x", 0o644, follow_symlinks=False)
As it happens, they're on Linux so they don't have lchmod() and their fchmodat() doesn't support AT_SYMLINK_NOFOLLOW. But! "x" isn't a symbolic link! In this case normal chmod would be fine fine.

How do we detect that the file is a symbolic link? That's easy, call lstat(). On Windows, if they gave us a wide path, call win32_lstat_w(). If they passed in a non-default dir_fd, call fstatat() (if available).

The one place where we can't fail over gracefully: os.stat() If we don't have the appropriate native stat function (lstat or fstatat), then obviously we can't stat nofollow the file to see if it's not a symbolic link and call normal stat(). Sad face.

The attached patch implements all of the above. It passes the regression test suite on Linux 64-bit (with and without pydebug) and Windows 32-bit (Debug and Release).

[hynek]

It also passes OS X.

There are no patch specific tests though. And alas, I don't have any platform at hand that would benefit from these additions. :(

The idea sounds good to me and the code LGTM though. However I can't really say much as I couldn't actually run it.

[pitrou]

I don't like this idea. Normally the system calls wrapped by the os module are fairly atomic. Here you're introducing the possibility for potentially nasty race conditions and exploits.

[larryhastings]

I think you're right. As Antoine pointed out in irc, for POSIX platforms, modules in os are almost exclusively atomic. This is a useful (if undocumented) feature from a security viewpoint, and we should not break it lightly.

Closing as wontfix. If someone thinks it's salvageable and wants to resurrect it, please discuss here.