bpo-32347: Emulate Libc copyfiles()'s st_flags logic on Darwin #4912
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
|
@rgov mind prefixing the PR title with Also you able to generate the news entry and fix AppVeyor? |
rgov
changed the title
|
I added the news entry under the macOS directory; should it go under Library? |
|
Yes |
|
Oh god look at this, 👀 looking at this file in expanded view it looks like some pep8 discrepencies needs fixing. I will fix that later in hopefully an trivial fix to as many standard library files I find having these issues I see. Also did you sign the CLA yet? |
|
I signed the CLA, is there something more I have to do? |
|
I dont think so other than wait for review. |
|
CLA is signed but I cannot review the PR personally -- I'm Linux user. |
|
Same, I am an Windows user though and cant run Darwin in VM due to bad CPU & GPU. But if I remember right @rhettinger uses MAC which is Darwin? |
|
I think I should have added |
Lib/shutil.py
Outdated
| if sys.platform == 'darwin': | ||
| # As of macOS 10.10, the `copyfile` API changed to not copy certain flags | ||
| mac_ver = tuple(int(x) for x in platform.mac_ver()[0].split('.')) | ||
| if mac_ver >= (10, 10, 0): |
There was a problem hiding this comment.
I'd compare against os.uname().release instead of using platform.mac_ver as this is related to a kernel API. The uname.release to compare against is 14.0.
Better yet: guard against the availability of os.SF_RESTRICTED instead.
|
|
||
| return (flags & ~omit_flags) | add_flags | ||
|
|
||
| if '_fix_flags' not in vars(): |
There was a problem hiding this comment.
Use "else:" instead
There was a problem hiding this comment.
If I used else:, it would make the code more complicated. If it matched with if sys.platform == 'darwin', then _fix_flags would not be defined for old versions of macOS. If I matched it with if mac_ver >= (10, 10, 0), then it would not be defined for other platforms. If I wanted to do both, I'd have to repeat code. This seemed the least complex.
Lib/shutil.py
Outdated
|
|
||
| # From xnu's bsd/sys/stat.h | ||
| UF_TRACKED = 0x00000040 | ||
| SF_RESTRICTED = 0x00080000 |
There was a problem hiding this comment.
These constants need to be to be added to Modules/posixmodule.c (like other related constants already are). Although this is problematic for the python.org installers as those are build on a 10.9 system and probably won't pick up these definitions when there are added to posixmodule.c.
There was a problem hiding this comment.
The related constants are in Lib/stat.py, not in posixmodule.c. Which is better?
| add_flags = 0 | ||
|
|
||
| # If the kernel automatically put SF_RESTRICTED on the destination | ||
| # already, we don't want to clear it |
There was a problem hiding this comment.
I'm not sure if this is needed, will macOS make files restricted when copying into special locations? The SIP feature only protects selected system files. Without this code the signature for this helper function could be simplified.
There was a problem hiding this comment.
The logic is the same as Apple's code, so I think it's better to keep it.
|
Ok, I made some of @ronaldoussoren's suggested changes and added a test case. I recommend accepting this change to address the regression on macOS, without getting too much into whether stat constants belong in stat.py or posixmodule.c. I've opted here to be consistent with other macOS-specific flag constants, but if they're all in the wrong place, then another patch should move them to posixmodule.c since it is a more significant change. |
|
This PR is stale because it has been open for 30 days with no activity. |
bpo-32347 https://bugs.python.org/issue32347
Apple's non-standard
copyfileAPI has behavior as of macOS 10.10 that drops some bits when copying flags from one file to another. One bit is related to System Integrity Protection and another seems related to iCloud document syncing. The first bit, SF_RESTRICTED, is tagged onto system files, and can only be set by a superuser. Thus, naïvely copying flags from a system file to a user file will attempt to set this bit and fail.This change implements the logic from
copyfileto drop these flags.I tested this on macOS 10.13.2 but I did not test on other platforms (including older versions of macOS).
I don't know how to do it myself, but it sounds like a good idea to just call the
copyfileAPI directly if possible, rather than re-implement its behavior.https://bugs.python.org/issue32347