Skip to content

Harmonize shutil.copymode() on Windows and POSIX #113188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
serhiy-storchaka opened this issue Dec 15, 2023 · 6 comments
Closed

Harmonize shutil.copymode() on Windows and POSIX #113188

serhiy-storchaka opened this issue Dec 15, 2023 · 6 comments
Labels
3.13 bugs and security fixes OS-windows

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Dec 15, 2023

The behavior of shutil.copymode() on Windows was changed by adding os.lchmod() in #59616. Now if follow_symlinks is false and both src and dst are symlinks, it copies the symlink mode (only the read-only bit). Previously it did nothing.

But there is still a difference between Windows and POSIX. If follow_symlinks is true and dst is a symlink, it sets the permission of the symlink on Windows (because os.chmod() still does not follow symlinks by default on Windows) and the permission of the target on POSIX.

shutil.copystat() has different behavior, because it passes the follow_symlinks argument explicitly.

I propose to harmonize shutil.copymode() behavior on Windows with POSIX and with shutil.copystat().

Linked PRs

@serhiy-storchaka serhiy-storchaka added OS-windows 3.13 bugs and security fixes labels Dec 15, 2023
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Dec 15, 2023
Previously it worked differenly if dst is a symbolic link:
it modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.

Also document similar changes in shutil.copystat() etc.
@serhiy-storchaka
Copy link
Member Author

I am not sure whether this change should be backported.

@zooba
Copy link
Member

zooba commented Dec 18, 2023

I am not sure whether this change should be backported.

I left a comment on the PR pointing out the logic is too complicated for me to see exactly what changed. However, I'd suggest that as this function doesn't really do much of interest on Windows (read only property is the only mode we support, and it's not a security attribute), chances are the only people using it are using it for POSIX and just glad it works elsewhere.

If they'll be glad that their non-readonly (rare) symlink to a (rare) readonly file now correctly copies the readonly attribute, then I'd say backport. (I maybe misunderstood the scenario, but I think the logic applies no matter, so probably backport after explaining the logic clearly in the docs.)

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Dec 19, 2023
Previously it worked differenly if dst is a symbolic link:
it modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Dec 19, 2023
Previously it worked differenly if dst is a symbolic link:
it modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.
@serhiy-storchaka
Copy link
Member Author

#113285 is a solution for 3.12.

@zooba
Copy link
Member

zooba commented Dec 19, 2023

Is there a specific reason we should fix copymode/copystat but not chmod in 3.12? Or just leave them all for 3.13 only?

@serhiy-storchaka
Copy link
Member Author

My main motivation is to avoid adding complicated versionchanged directives and What's New entry. They are not needed for bugfix.

Backporting changes in chmod does not help much without backporting the addition of os.lchmod, because some code tests its existency. And this is further in a "new feature" territory. If do not backport os.lchmod, the code will be of similar complexity to #113285.

@zooba
Copy link
Member

zooba commented Dec 22, 2023

Fair enough. Backporting the fix for the high level function seems okay to me now.

serhiy-storchaka added a commit that referenced this issue Dec 23, 2023
…ows (GH-113285)

Previously they worked differenly if dst is a symbolic link:
they modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 23, 2023
…n Windows (pythonGH-113285)

Previously they worked differenly if dst is a symbolic link:
they modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.
(cherry picked from commit c7874bb)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit that referenced this issue Dec 23, 2023
…ows (GH-113285) (GH-113426)

Previously they worked differenly if dst is a symbolic link:
they modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.
(cherry picked from commit c7874bb)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit that referenced this issue Dec 23, 2023
Previously it worked differently if dst is a symbolic link:
it modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.

Also document similar changes in shutil.copystat().
ryan-duve pushed a commit to ryan-duve/cpython that referenced this issue Dec 26, 2023
Previously it worked differently if dst is a symbolic link:
it modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.

Also document similar changes in shutil.copystat().
kulikjak pushed a commit to kulikjak/cpython that referenced this issue Jan 22, 2024
Previously it worked differently if dst is a symbolic link:
it modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.

Also document similar changes in shutil.copystat().
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
Previously it worked differently if dst is a symbolic link:
it modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.

Also document similar changes in shutil.copystat().
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Previously it worked differently if dst is a symbolic link:
it modified the permission bits of dst itself rather than the file
it points to if follow_symlinks is true or src is not a symbolic link,
and did nothing if follow_symlinks is false and src is a symbolic link.

Also document similar changes in shutil.copystat().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes OS-windows
Projects
None yet
Development

No branches or pull requests

2 participants