bpo-31940: Reject faulty lchmod implementations #4783

asottile · 2017-12-10T19:37:05Z

This is a runtime attempt at implementing #4267

A few notes:

During review it was suggested to cache the failed lchmod attempts, however I don't think this is doable:
- lchmod is used for both symlinks and not symlinks if follow_symlinks=False -- it works fine for normal files
- detecting whether something is a symlink up front throws away all benefits of caching whether it fails as you still make a syscall
- the code for fchmodat adjacent to the lchmod code serves as precedent for both of these decisions

https://bugs.python.org/issue31940

fruch · 2017-12-11T05:56:43Z

@asottile nice, can't understand how there's a coredump in the test_shutil.py (under clang).
you tested this one only under alpine ?

asottile · 2017-12-11T06:08:15Z

@fruch woops, forgot an else -- silly macro trickery got me again :(

asottile · 2017-12-11T17:55:17Z

Please don't merge this yet, it depends on changes made in #4797

asottile · 2017-12-14T17:02:01Z

I've rebased this now that #4797 is merged, should be ready for review again

Lib/test/test_shutil.py

vstinner · 2017-12-15T14:36:14Z

Modules/posixmodule.c

Oh, there are two different error codes? Which one is raised on muslc?

muslc raises EOPNOTSUPP (errno 95) though on my platform they were #defined to the same thing. I simply picked the same errnos as fchmodat catches below -- I suspect implementations use fchmodat to implement lchown (or the other way around).

asottile · 2017-12-29T19:27:21Z

@vstinner would love another review :) no rush though!

asottile · 2018-02-11T21:43:49Z

@vstinner friendly ping :)

asottile · 2018-02-20T22:32:09Z

Looking through devguide -- maybe @giampaolo can review the shutil changes?

upstream issue: https://bugs.python.org/issue31940 There are two PR's proposed to fix this, but both seem to be stalling waiting for review. I previously used what appears to be the favored of the two approaches[1] to fix this, with plan of keeping it musl-only until PR was merged. However, while writing up a commit message explaining the problem and why it needed fixing... I investigated a bit and found it increasingly hard to justify anything other than ... simply not using lchmod. Here's what I found: * lchmod is non-POSIX, seems BSD-only these days * Functionality of lchmod isn't supported on Linux * best scenario on Linux would be an error * POSIX does provide lchmod-esque functionality with fchmodat(), which AFAICT is generally preferred. * Python intentionally overlooks fchmodat()[2] electing instead to use lchmod() behavior as a proxy for whether fchmodat() "works". I'm not sure I follow their reasoning... * both glibc and musl provide lchmod impls: * glibc returns ENOSYS "not implemented" * musl implements lchmod with fchmodat(), and so returns EOPNOTSUPP "op not supported" * Python doesn't expect EOPNOTSUPP from lchmod, since it's not valid on BSD's lchmod. * "configure" doesn't actually check lchmod usefully, instead checks for glibc preprocessor defines to indicate if the function is just a stub[3]; somewhat fittingly, if the magic macros are defined then the next line of the C source is "choke me", causing the compiler to trip, fall, and point a finger at whatever is near where it ends up. (somewhat amusing, but AFAIK effective way to get an error :P) I'm leaving out links to threads on mailing lists and such, but for now I hope I've convinced you (or to those reading commit history: explained my reasons) that this is a bit of a mess[4]. And so instead of making a big mess messier, and with hopes of never thinking about this again, I propose we simply tell Python "don't use lchmod" on Linux. [1] python/cpython#4783 [2] https://github.com/python/cpython/blob/28453feaa8d88bbcbf6d834b1d5ca396d17265f2/Lib/os.py#L144 [3] https://github.com/python/cpython/blob/28453feaa8d88bbcbf6d834b1d5ca396d17265f2/configure#L2198 [4] Messes happen, no good intention goes unpunished :).

TheKevJames · 2018-08-17T22:17:12Z

Lib/test/test_shutil.py

+    if not support.can_symlink() or not hasattr(os, 'lchmod'):
+        return False
+
+    fname = tempfile.mktemp()


Should this be mkstemp? mktemp seems to be deprecated.

This was copied from adjacent test (in the original PR). It's not exactly that mktemp is deprecated, it's that it's insecure. However in this case, the same security problem would exist since it's essentially:

mkstemp

unlink the file made by mkstemp

symlink to the location made by mkstemp

There's a race between 2 and 3 where something could create a file

Fortunately it doesn't really matter since it's just a test.

asottile · 2018-11-26T17:29:40Z

Almost this branch's birthday -- rebased to resolve conflicts

fruch · 2018-11-26T17:35:56Z

@asottile anything I could do to help ?

asottile · 2018-11-26T17:38:39Z

@fruch iirc an approving review (even from an outsider) will move the state from [awaiting review] to [awaiting core review] which might (???) gather more attention from core devs

TheKevJames

Changeset LGTM, but I have not worked with this chunk of the cpython codebase before. Would be great to get a core contributor review as well!

jdubost · 2018-11-27T13:11:19Z

Dunno if this can help but i encountered the issue (https://bugs.python.org/issue31940) with a python-3.6 tox environment within an alpine image python:2.7-alpine3.6 (+ python-3.6.5 installed manually)
Then i upgraded to a python:3.6.7-alpine3.6 image (python-3.6.7 native) -> it solved the issue.

Since tox on a python2 environment works well on python:3.6.7-alpine3.6, i now use this image for all my tests.

Edit :

python:3.6.7-alpine3.6 + python downgrade to 3.6.5 : NOK
python:3.6.7-alpine3.6 : OK
python:2.7-alpine3.6 + python 3.6.5 : NOK

asottile · 2018-11-27T17:50:06Z

@jdubost looks like alpine is working around this bug by manually turning off HAVE_LCHMOD:

python:3.6.7-alpine3.6$ grep HAVE_LCHMOD /usr/local/include/python3.6m/pyconfig.h 
/* #undef HAVE_LCHMOD */

python:3.6.1-alpine3.6$ grep HAVE_LCHMOD /usr/local/include/python3.6m/pyconfig.h 
#define HAVE_LCHMOD 1

It looks like @benjaminp forced this to off on all linuxes in 40caa05

which looks eerily similar to my rejected patch in #4267 😭

benjaminp · 2018-11-28T04:57:13Z

Hmm, I didn't see any of this.

I don't understand the objection to a compile-time check. Whether you're targeting Linux or not is something you must know at compile time. It would be one thing if an operating system existed that sometimes returned ENOSYS from lchmod. But, I know of no such system.

asottile · 2018-11-28T05:30:38Z

unclear as well. I guess since this is solved I can close this and we can resolve the linked issue.

I think some of the changes in the patch are useful so I'll make a separate bpo and propose them (notably, os.stat / os.chmod are now always present so the lookup(...) calls are unnecessary)

the-knights-who-say-ni added the CLA signed label Dec 10, 2017

bedevere-bot added the awaiting review label Dec 10, 2017

asottile mentioned this pull request Dec 10, 2017

bpo-31940: Reject faulty lchown implementations #4267

Closed

asottile force-pushed the bpo-31940-ignore-faulty-lchmod-implementations branch from 85ba86a to d16cd68 Compare December 11, 2017 06:08

asottile force-pushed the bpo-31940-ignore-faulty-lchmod-implementations branch from d16cd68 to 5ed4b99 Compare December 11, 2017 06:23

asottile force-pushed the bpo-31940-ignore-faulty-lchmod-implementations branch 2 times, most recently from a735a41 to 130279d Compare December 14, 2017 17:01

vstinner reviewed Dec 15, 2017

View reviewed changes

asottile force-pushed the bpo-31940-ignore-faulty-lchmod-implementations branch 3 times, most recently from 86425d2 to d64773b Compare December 15, 2017 15:36

jacobsvante mentioned this pull request Dec 15, 2017

[3-alpine] shutil.copytree fail to copy a tree with a broken symbolic link docker-library/python#155

Closed

asottile force-pushed the bpo-31940-ignore-faulty-lchmod-implementations branch from d64773b to 2e57192 Compare December 15, 2017 17:58

jacobsvante added a commit to jacobsvante/python that referenced this pull request Dec 15, 2017

Fix with code from pull request python/cpython#4783 applied

d34c035

asottile mentioned this pull request Feb 20, 2018

shutil.Error while installing hooks (alpine docker, musl) pre-commit/pre-commit#655

Closed

dtzWill mentioned this pull request Apr 26, 2018

cpython: don't use lchmod() on Linux, fix w/musl NixOS/nixpkgs#39516

Closed

8 tasks

dtzWill mentioned this pull request Apr 26, 2018

cpython: don't use lchmod() on Linux, fix w/musl NixOS/nixpkgs#39517

Merged

8 tasks

TheKevJames reviewed Aug 17, 2018

View reviewed changes

asottile force-pushed the bpo-31940-ignore-faulty-lchmod-implementations branch from 2e57192 to 46ea965 Compare November 26, 2018 17:29

TheKevJames approved these changes Nov 26, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Nov 26, 2018

bpo-31940: Reject faulty lchmod implementations

3460a76

asottile force-pushed the bpo-31940-ignore-faulty-lchmod-implementations branch from 46ea965 to 3460a76 Compare November 27, 2018 17:34

asottile closed this Nov 28, 2018

asottile deleted the bpo-31940-ignore-faulty-lchmod-implementations branch November 28, 2018 05:30

Uh oh!

bpo-31940: Reject faulty lchmod implementations #4783

bpo-31940: Reject faulty lchmod implementations #4783

Uh oh!

Conversation

asottile commented Dec 10, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fruch commented Dec 11, 2017

Uh oh!

asottile commented Dec 11, 2017

Uh oh!

asottile commented Dec 11, 2017

Uh oh!

asottile commented Dec 14, 2017

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

asottile Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

asottile commented Dec 29, 2017

Uh oh!

asottile commented Feb 11, 2018

Uh oh!

asottile commented Feb 20, 2018

Uh oh!

TheKevJames Aug 17, 2018

Choose a reason for hiding this comment

Uh oh!

asottile Aug 18, 2018

Choose a reason for hiding this comment

Uh oh!

asottile commented Nov 26, 2018

Uh oh!

fruch commented Nov 26, 2018

Uh oh!

asottile commented Nov 26, 2018

Uh oh!

TheKevJames left a comment

Choose a reason for hiding this comment

Uh oh!

jdubost commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Nov 27, 2018

Uh oh!

benjaminp commented Nov 28, 2018

Uh oh!

asottile commented Nov 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

asottile commented Dec 10, 2017 •

edited by bedevere-bot

Loading

jdubost commented Nov 27, 2018 •

edited

Loading