bpo-31940: Reject faulty lchown implementations #4267

asottile · 2017-11-04T04:39:28Z

Verified by compiling with alpine using the following Dockerfile and test script:

FROM alpine
RUN apk update && \
    apk add expat-dev libressl-dev zlib-dev ncurses-dev bzip2-dev xz-dev \
        sqlite-dev libffi-dev tcl-dev linux-headers gdbm-dev readline-dev \
        gcc nano bash git python3 alpine-sdk

#!/usr/bin/env bash
set -euxo pipefail
docker build -t python-build .
docker run \
    --rm -ti \
    -v "$PWD/cpython:/src:ro" \
    python-build bash -exc '\
        cp -r /src /tmp/src && \
        cd /tmp/src && \
        ./configure || (cat config.log && exit 1) && \
        make -j8 && \
        ./python -m test.test_shutil \
    '

Before

+ ./python -m test.test_shutil
..................s.................Es.E.....EssEE.....EE..ss..........s.................Fs......s.s..
======================================================================
ERROR: test_copy2_symlinks (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 531, in test_copy2_symlinks
    os.lchmod(src_link, stat.S_IRWXU | stat.S_IRWXO)
OSError: [Errno 95] Not supported: '/tmp/tmpcfxc61cw/baz'

======================================================================
ERROR: test_copy_symlinks (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 508, in test_copy_symlinks
    os.lchmod(src_link, stat.S_IRWXU | stat.S_IRWXO)
OSError: [Errno 95] Not supported: '/tmp/tmplde2crj7/baz'

======================================================================
ERROR: test_copymode_symlink_to_symlink (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 318, in test_copymode_symlink_to_symlink
    os.lchmod(src_link, stat.S_IRWXO|stat.S_IRWXG)
OSError: [Errno 95] Not supported: '/tmp/tmpr3v0c24m/baz'

======================================================================
ERROR: test_copystat_symlinks (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 364, in test_copystat_symlinks
    os.lchmod(src_link, stat.S_IRWXO)
OSError: [Errno 95] Not supported: '/tmp/tmplqh68wha/baz'

======================================================================
ERROR: test_copytree_dangling_symlinks (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 892, in test_copytree_dangling_symlinks
    shutil.copytree(src_dir, dst_dir, symlinks=True)
  File "/tmp/src/Lib/shutil.py", line 359, in copytree
    raise Error(errors)
shutil.Error: [('/tmp/tmpj76xsejs/test.txt', '/tmp/tmpg6s1ihql/destination3/test.txt', "[Errno 95] Not supported: '/tmp/tmpg6s1ihql/destination3/test.txt'")]

======================================================================
ERROR: test_copytree_symlink_dir (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 911, in test_copytree_symlink_dir
    shutil.copytree(src_dir, dst_dir, symlinks=True)
  File "/tmp/src/Lib/shutil.py", line 359, in copytree
    raise Error(errors)
shutil.Error: [('/tmp/tmpsa0vkix_/link_to_dir', '/tmp/tmpta37k_s9/destination2/link_to_dir', "[Errno 95] Not supported: '/tmp/tmpta37k_s9/destination2/link_to_dir'")]

======================================================================
ERROR: test_copytree_symlinks (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 651, in test_copytree_symlinks
    os.lchmod(src_link, stat.S_IRWXU | stat.S_IRWXO)
OSError: [Errno 95] Not supported: '/tmp/tmphrkx18bj/src/sub/link'

======================================================================
FAIL: test_unzip_zipfile (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 1118, in test_unzip_zipfile
    subprocess.check_output(zip_cmd, stderr=subprocess.STDOUT)
subprocess.CalledProcessError: Command '['unzip', '-t', '/tmp/tmppyfabhkq/archive.zip']' returned non-zero exit status 1.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 1122, in test_unzip_zipfile
    self.fail(msg.format(exc, details))
AssertionError: Command '['unzip', '-t', '/tmp/tmppyfabhkq/archive.zip']' returned non-zero exit status 1.

**Unzip Output**
unzip: unrecognized option: t
BusyBox v1.26.2 (2017-10-04 13:37:41 GMT) multi-call binary.

Usage: unzip [-lnopq] FILE[.zip] [FILE]... [-x FILE...] [-d DIR]

Extract FILEs from ZIP archive

	-l	List contents (with -q for short form)
	-n	Never overwrite files (default: ask)
	-o	Overwrite
	-p	Print to stdout
	-q	Quiet
	-x FILE	Exclude FILEs
	-d DIR	Extract into DIR


----------------------------------------------------------------------
Ran 102 tests in 0.289s

FAILED (failures=1, errors=7, skipped=10)

After

+ ./python -m test.test_shutil
..................s..................s.......s.s...........ss..........s.................Fs......s.s..
======================================================================
FAIL: test_unzip_zipfile (__main__.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 1118, in test_unzip_zipfile
    subprocess.check_output(zip_cmd, stderr=subprocess.STDOUT)
subprocess.CalledProcessError: Command '['unzip', '-t', '/tmp/tmp9zf_mymx/archive.zip']' returned non-zero exit status 1.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/src/Lib/test/test_shutil.py", line 1122, in test_unzip_zipfile
    self.fail(msg.format(exc, details))
AssertionError: Command '['unzip', '-t', '/tmp/tmp9zf_mymx/archive.zip']' returned non-zero exit status 1.

**Unzip Output**
unzip: unrecognized option: t
BusyBox v1.26.2 (2017-10-04 13:37:41 GMT) multi-call binary.

Usage: unzip [-lnopq] FILE[.zip] [FILE]... [-x FILE...] [-d DIR]

Extract FILEs from ZIP archive

	-l	List contents (with -q for short form)
	-n	Never overwrite files (default: ask)
	-o	Overwrite
	-p	Print to stdout
	-q	Quiet
	-x FILE	Exclude FILEs
	-d DIR	Extract into DIR


----------------------------------------------------------------------
Ran 102 tests in 0.253s

FAILED (failures=1, skipped=10)

https://bugs.python.org/issue31940

fruch

Can't argue with running tests :)

vstinner

I dislike checking once in configure, since the platform used to build Python can be different than the platform used to run Python. There is a similar issue for Linux vendors using different kernel versions to build packages than the kernel used in the latest version of the distro.

IMHO the right fix is to check at runtime (in Python, not in configure) if os.lchmod() works: a check in shutil and another check in test_shutil (to skip tests).

bedevere-bot · 2017-11-06T22:56:43Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

fruch · 2017-11-06T23:15:28Z

@Haypo can you point us in the direction of such type of runtime checks ? are there existing similar runtime checks as you're suggesting ?

asottile · 2017-11-06T23:17:03Z

@fruch I think I can do something similar to what I proposed here: https://bugs.python.org/issue31940#msg305528

just need to fix up the tests to not use lchown when it's broken -- I think I can follow some examples in related tests

vstinner · 2017-11-06T23:27:40Z

@Haypo can you point us in the direction of such type of runtime checks ?

Some examples:

time.process_time() tries getrusage(), times() or falls back on clock()
open() tries to use the atomic O_CLOEXEC flag, ioctl(FIOCLEX), or falls back on fnctl(F_GETFD)+fnctl(F_SETFD)
os.dup() tries to call dup2(O_CLOEXEC), or falls back on dup()+make non-inheritable
os.pipe() tries to call pipe2(O_CLOEXEC), or falls back on pipe()+make non-inheritable
... : there are many other examples

fruch · 2017-11-06T23:32:31Z

if lchown it's not working on alpine as expected, why keep using it ? I guess it can fail in more use cases. and disabling in compile time sounds a safer option to me.

skipping tests mean some things are not working on alpine, and no one would attended to it, cause all tests are passing...

vstinner · 2017-11-06T23:36:26Z

if lchown it's not working on alpine as expected, why keep using it ?

You may build Python on Debian with a working lchmod and run it on Alpine which has a broken lchmod: bug.

You may build Python on Alphine with a broken lchmod and run it on Debian which has a working lchmod: if you disable lchmod, you miss the feature on Debian.

configure is not the right place to implement the check. It should be done at runtime.

The os module is designed to a thin wrapper of OS functions. shutil is more a high-level module handling low-level stuff for the user.

asottile · 2017-11-06T23:42:28Z

@Haypo that actually doesn't work today, for example on ubuntu xenial (debian) there is no lchmod, so compiling on alpine and then running on xenial I get linking errors for lchmod (among other things) (because alpine uses musl and not glibc)

That said, I'm looking for ways to improve this that don't involve configure -- I'm reasonably close.

asottile · 2017-11-06T23:57:38Z

Notably this block probably also need to be applied to lchmod:

cpython/Modules/posixmodule.c

Lines 2794 to 2817 in ad455cd

    
           #ifdef HAVE_FCHMODAT 
        
               if ((dir_fd != DEFAULT_DIR_FD) || !follow_symlinks) { 
        
                   /* 
        
                    * fchmodat() doesn't currently support AT_SYMLINK_NOFOLLOW! 
        
                    * The documentation specifically shows how to use it, 
        
                    * and then says it isn't implemented yet. 
        
                    * (true on linux with glibc 2.15, and openindiana 3.x) 
        
                    * 
        
                    * Once it is supported, os.chmod will automatically 
        
                    * support dir_fd and follow_symlinks=False.  (Hopefully.) 
        
                    * Until then, we need to be careful what exception we raise. 
        
                    */ 
        
                   result = fchmodat(dir_fd, path->narrow, mode, 
        
                                     follow_symlinks ? 0 : AT_SYMLINK_NOFOLLOW); 
        
                   /* 
        
                    * But wait!  We can't throw the exception without allowing threads, 
        
                    * and we can't do that in this nested scope.  (Macro trickery, sigh.) 
        
                    */ 
        
                   fchmodat_nofollow_unsupported = 
        
                                    result && 
        
                                    ((errno == ENOTSUP) || (errno == EOPNOTSUPP)) && 
        
                                    !follow_symlinks; 
        
               } 
        
               else

vstinner · 2017-11-07T00:36:47Z

I suggest to leave this PR unchanged, since I'm not sure anymore that a runtime check is the best option (see the discussion on the issue).

If you want to implement runtime checks, please open a new PR.

asottile · 2017-11-07T01:15:14Z

@Haypo cool, I'm going to wait to pursue that further until I hear back on the issue, in the meantime, here's the start of that idea (passing the same set of tests, but there's much more that can be done I assume): asottile@61095aa

fruch · 2017-11-07T05:54:37Z

seems like you guys were busy, while I was sleeping :)

@Haypo, thanks for keeping an open mind, I'm impressed.

canadian/cross compiling is a major pain (I come from a background of lots of embedded work). and I think that distribution/package owner that will try those types of crazy mixtures is bound to a world of pain. (lchmod will be the least of his problems)

I'll put my vote on users experience over "crazy" distribution/package owners.

even that in cases of linux headers and versions it's can be a tricker question as you suggest, cause user can do all kinds of mixtures there.

But in our case users can't swap glibc version, or even implementations of libc that easily.

vstinner · 2017-11-07T14:56:31Z

I'll put my vote on users experience over "crazy" distribution/package owners.

My examples are not theorical, but issues that I saw in practice. People are already doing that. Maybe not with different Linux distributions, but at least with different versions of the same distribution.

Another example: we provide a single binary for all Windows versions, so we load some functions at runtime and check if there are available on the current Windows versions. Python also checks the Windows version at many places.

Another example: we also provide a single binary for all macOS versions. For example, the poll() function of macOS is regulary broken, so we implement some checks at runtime...

rm-you · 2017-11-17T11:22:23Z

I just started seeing this same issue on Gentoo+musl as well. It's quite fortuitous for me that this thread was already ongoing!

So, are folks still deciding whether to do run-time or compile-time checks?
Personally I see the advantages of both, though having done some cross-compile work as well, my thoughts are:

I am already expecting to having to fiddle with things to get them to work right when cross-compiling.
I think cross-compiling is probably a 1% use-case. For the normal user (99% of the time) it's going to be "compile and run locally".
Checking repeatedly at run-time seems like a waste of cycles (albeit very few).

So, I think I'd vote for compile-time as well.

fruch · 2017-11-17T12:02:49Z

I think @vstinner already agreed to first do a compile time, and make the runtime in different PR

fruch · 2017-11-17T12:05:36Z

what is the next step ? we need someone from core-dev to approve ?

rm-you · 2017-11-18T04:32:52Z

Looks like if @vstinner was OK with it as-is for now, we need their review status changed?

asottile · 2017-11-18T04:40:50Z

I think the idea was to leave this one open for consideration (in case others agreed that a compile time check is ok) and make a separate runtime PR (which'll actually get included) -- I haven't had time to finish my runtime approach yet though I've gotten encouraging remarks on the WIP commit: asottile@61095aa

rm-you

My comments are about docs and style, I think the code is fine (though in some cases really hard for me to parse due to lack of indentation).

rm-you · 2017-11-18T04:33:55Z

Misc/NEWS.d/next/Build/2017-11-03-21-54-32.bpo-31940.fG1wnQ.rst

@@ -0,0 +1,2 @@
+Detect faulty ``lchmod`` implementation to fix ``shutil.copystat`` on alpine


It might be useful to say "on Alpine linux and other distributions using musl libc", because the issue is actually related to the musl implementation, I believe. This affects me on Gentoo+musl.

yep, I did hit this on gentoo musl as well

oh shoot, I totally forgot about this patch -- I need to revive my other approach 😨

rm-you · 2017-11-18T04:34:29Z

configure

 fi

+
+# on alpine, lchmod raises errno EOPNOTSUPP on symlinks


Again, technically not just Alpine, it's a musl libc thing.

rm-you · 2017-11-18T04:37:40Z

configure

+else
+  if test "$cross_compiling" = yes; then :
+  ac_cv_have_lchmod=cross
+else


Is this else for the inner if-block? Should it be indented one level, along with an extra level for the cat command below it?

Actually, It looks like there's a ton of nested stuff below that isn't indented -- it makes it hard to read, but I don't know if that is a style thing I'm not used to since I haven't worked specifically in configure files much...

configure is entirely machine generated, the actual code is in configure.am

rm-you · 2017-11-18T04:41:09Z

configure

+
+fi
+if test "$ac_cv_have_lchmod" = yes ; then
+


Extra newlines for a reason? Also, not indented?

rm-you · 2017-11-18T04:41:31Z

configure.ac

 fi

+
+# on alpine, lchmod raises errno EOPNOTSUPP on symlinks


Another mention of just Alpine, same suggestion as above.

rm-you · 2017-11-18T04:44:15Z

@asottile Ok, that wasn't quite how I read it, but I guess that's fine. Honestly either one will solve the problem, and if you're willing to get the work done for the other method then hooray! 🎉

fruch · 2017-12-10T18:33:54Z

@asottile any news on this one ?

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

Here's the other attempt at this: #4783

asottile · 2018-01-04T19:56:56Z

Figured I'd bump here too since the other PR hasn't gotten any attention. Here's the runtime implementation of this: #4783 -- a review on that would help greatly with moving this forward :)

asottile · 2018-11-26T17:30:30Z

Closing this in favor of #4783 -- please review that!

the-knights-who-say-ni added the CLA signed label Nov 4, 2017

bedevere-bot added the awaiting review label Nov 4, 2017

bpo-31940: Reject faulty lchown implementations

ad495d0

asottile force-pushed the fix_shutil_copystat_alpine_bpo-31940 branch from 54aeb10 to ad495d0 Compare November 4, 2017 04:54

This was referenced Nov 4, 2017

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

Closed

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

Closed

fruch approved these changes Nov 4, 2017

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Nov 4, 2017

vstinner requested changes Nov 6, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Nov 6, 2017

rm-you reviewed Nov 18, 2017

View reviewed changes

yuvipanda mentioned this pull request Dec 1, 2017

Alpine Linux, pip/python & symlinks together break our build jupyterhub/repo2docker#160

Closed

asottile mentioned this pull request Dec 10, 2017

bpo-31940: Reject faulty lchmod implementations #4783

Closed

asottile closed this Nov 26, 2018

asottile deleted the fix_shutil_copystat_alpine_bpo-31940 branch November 26, 2018 17:30

		@@ -0,0 +1,2 @@
		Detect faulty ``lchmod`` implementation to fix ``shutil.copystat`` on alpine

Uh oh!

bpo-31940: Reject faulty lchown implementations #4267

bpo-31940: Reject faulty lchown implementations #4267

Uh oh!

Conversation

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

Uh oh!

Before

After

Uh oh!

fruch left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 6, 2017

Uh oh!

fruch commented Nov 6, 2017

Uh oh!

asottile commented Nov 6, 2017

Uh oh!

vstinner commented Nov 6, 2017

Uh oh!

fruch commented Nov 6, 2017

Uh oh!

vstinner commented Nov 6, 2017

Uh oh!

asottile commented Nov 6, 2017

Uh oh!

asottile commented Nov 6, 2017

Uh oh!

vstinner commented Nov 7, 2017

Uh oh!

asottile commented Nov 7, 2017

Uh oh!

fruch commented Nov 7, 2017

Uh oh!

vstinner commented Nov 7, 2017

Uh oh!

rm-you commented Nov 17, 2017

Uh oh!

fruch commented Nov 17, 2017

Uh oh!

fruch commented Nov 17, 2017

Uh oh!

rm-you commented Nov 18, 2017

Uh oh!

asottile commented Nov 18, 2017

Uh oh!

rm-you left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rm-you commented Nov 18, 2017

Uh oh!

fruch commented Dec 10, 2017

Uh oh!

asottile commented Dec 10, 2017

Uh oh!

asottile commented Jan 4, 2018

Uh oh!

asottile commented Nov 26, 2018

Uh oh!

Reviewers

asottile commented Nov 4, 2017 •

edited by bedevere-bot

Loading