gh-91133: tempfile.TemporaryDirectory: fix symlink bug in cleanup by kwi-dk · Pull Request #99930 · python/cpython

kwi-dk · 2022-12-01T16:07:07Z

This PR fixes issue gh-91133, in which TemporaryDirectory.cleanup could try to fix permissions when deleting a symlink, but accidentally fix permissions of the target of the symlink instead.

~~(It also changes the test_flags test case so it doesn't leave behind NOUNLINK files when the test fails, which is just annoying when working on any change to the TemporaryDirectory code.)~~

Compatibility: There could conceivably be code out there relying on TemporaryDirectory.cleanup modifying permissions on files outside the tempdir being cleaned up, which the fixed code will no longer do.

ghost · 2022-12-01T16:07:41Z

All commit authors signed the Contributor License Agreement.

During development, it becomes tiresome to have to manually clean up these files in case of unrelated TemporaryDirectory breakage.

netlify · 2022-12-14T17:50:21Z

✅ Deploy Preview for python-cpython-preview ready!

Name	Link
🔨 Latest commit	`db40615`
🔍 Latest deploy log	https://app.netlify.com/sites/python-cpython-preview/deploys/639b60bbd252c20008d29de1
😎 Deploy Preview	https://deploy-preview-99930--python-cpython-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

carljm

This looks to me like a good fix, thanks! A few comments.

Misc/NEWS.d/next/Library/2022-12-01-16-57-44.gh-issue-91133.LKMVCV.rst

Lib/test/test_tempfile.py

This reverts commit d8f50e5.

Co-authored-by: Carl Meyer <[email protected]>

carljm

LGTM modulo one more comment. Thanks!

Lib/test/test_tempfile.py

arhadthedev · 2023-02-28T12:45:51Z

@serhiy-storchaka and @vstinner (as active committers into tempfile.py)

serhiy-storchaka

LGTM, but I prefer to test with real filesystem. Mock tests are too fragile, they depend on implementation details that can be changed.

vstinner · 2023-12-07T07:57:43Z

Lib/tempfile.py


    @classmethod
    def _rmtree(cls, name, ignore_errors=False):
+        def without_following_symlinks(func, path, *args):


I suggest to rename the function to "no_follow_symlinks" or "dont_follow_symlinks".

I dislike this trend to declare nested functions. Can't you move these functions at the module level, just make them private? This function doesn't use name nor ignore_errors.

bedevere-app · 2023-12-07T16:04:20Z

GH-112838 is a backport of this pull request to the 3.12 branch.

miss-islington-app · 2023-12-07T16:04:20Z

Sorry, @kwi-dk and @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 81c16cd94ec38d61aa478b9a452436dc3b1b524d 3.11

…up (pythonGH-99930) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

miss-islington-app · 2023-12-07T16:04:33Z

Sorry, @kwi-dk and @serhiy-storchaka, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 81c16cd94ec38d61aa478b9a452436dc3b1b524d 3.10

miss-islington-app · 2023-12-07T16:04:43Z

Sorry, @kwi-dk and @serhiy-storchaka, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 81c16cd94ec38d61aa478b9a452436dc3b1b524d 3.9

miss-islington-app · 2023-12-07T16:04:54Z

Sorry, @kwi-dk and @serhiy-storchaka, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 81c16cd94ec38d61aa478b9a452436dc3b1b524d 3.8

…n cleanup (pythonGH-99930) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-app · 2023-12-07T16:07:47Z

GH-112839 is a backport of this pull request to the 3.11 branch.

afeblot · 2023-12-07T16:09:13Z

I opened the issue this is fixing. Thank you all for the fix.

bedevere-app · 2023-12-07T16:12:54Z

GH-112840 is a backport of this pull request to the 3.10 branch.

…n cleanup (pythonGH-99930) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-app · 2023-12-07T16:17:23Z

GH-112842 is a backport of this pull request to the 3.9 branch.

… cleanup (pythonGH-99930) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-app · 2023-12-07T16:20:41Z

GH-112843 is a backport of this pull request to the 3.8 branch.

… cleanup (pythonGH-99930) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

…nup (GH-99930) (GH-112838) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

…nup (GH-99930) (GH-112839) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]>

vstinner · 2023-12-07T23:15:54Z

Thanks @kwi-dk for this fix. Thanks @serhiy-storchaka to taking the lead to update the code and merge the fix!

…up (GH-99930) (GH-112843) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]>

…up (GH-99930) (GH-112842) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]>

…nup (GH-99930) (GH-112840) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]>

…up (pythonGH-99930) Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot mentioned this pull request Dec 1, 2022

tempfile.TemporaryDirectory fails removing dir in some edge cases related to symlinks [CVE-2023-6597] #91133

Closed

bedevere-bot added the awaiting review label Dec 1, 2022

kwi-dk added 3 commits December 14, 2022 18:50

tempfile tests: try not to leave NOUNLINK files behind

d8f50e5

During development, it becomes tiresome to have to manually clean up these files in case of unrelated TemporaryDirectory breakage.

pythongh-91133: tempfile.TemporaryDirectory: fix symlink bug in cleanup

d268947

NEWS: fix blurb typo

60de13a

kwi-dk force-pushed the fix-91133 branch from c4a1471 to 60de13a Compare December 14, 2022 17:50

carljm reviewed Dec 14, 2022

View reviewed changes

Misc/NEWS.d/next/Library/2022-12-01-16-57-44.gh-issue-91133.LKMVCV.rst Outdated Show resolved Hide resolved

Lib/test/test_tempfile.py Outdated Show resolved Hide resolved

Lib/test/test_tempfile.py Outdated Show resolved Hide resolved

kwi-dk and others added 2 commits December 15, 2022 12:02

Revert "tempfile tests: try not to leave NOUNLINK files behind"

983a9ac

This reverts commit d8f50e5.

Apply suggestions from code review

5b83afe

Co-authored-by: Carl Meyer <[email protected]>

carljm reviewed Dec 15, 2022

View reviewed changes

Lib/test/test_tempfile.py Outdated Show resolved Hide resolved

Run test even when chmod doesn't support follow_symlinks

db40615

carljm approved these changes Dec 15, 2022

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Dec 15, 2022

Merge branch 'main' into fix-91133

96124ef

arhadthedev added the stdlib Standard Library Python modules in the Lib/ directory label Feb 28, 2023

serhiy-storchaka self-requested a review December 5, 2023 20:04

serhiy-storchaka added 2 commits December 6, 2023 11:53

Merge branch 'main' into fix-91133

1aadbdd

Rewrite tests.

cb9aea4

serhiy-storchaka approved these changes Dec 6, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Dec 6, 2023

serhiy-storchaka added 2 commits December 6, 2023 21:32

Fix on Windows.

f106e25

Add a test for file flags.

0be03d1

vstinner reviewed Dec 7, 2023

View reviewed changes

serhiy-storchaka added 2 commits December 7, 2023 11:24

Address review comments.

0a05de0

Merge branch 'main' into fix-91133

081a898

bedevere-app bot removed the awaiting merge label Dec 7, 2023

bedevere-app bot removed the needs backport to 3.12 only security fixes label Dec 7, 2023

miss-islington-app bot assigned serhiy-storchaka Dec 7, 2023

bedevere-app bot removed the needs backport to 3.11 only security fixes label Dec 7, 2023

bedevere-app bot removed the needs backport to 3.10 only security fixes label Dec 7, 2023

bedevere-app bot removed the needs backport to 3.9 label Dec 7, 2023

bedevere-app bot removed the needs backport to 3.8 label Dec 7, 2023

serhiy-storchaka added a commit that referenced this pull request Dec 7, 2023

[3.11] gh-91133: tempfile.TemporaryDirectory: fix symlink bug in clea…

5585334

…nup (GH-99930) (GH-112839) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]>

ambv pushed a commit that referenced this pull request Jan 17, 2024

[3.8] gh-91133: tempfile.TemporaryDirectory: fix symlink bug in clean…

02a9259

…up (GH-99930) (GH-112843) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]>

ambv pushed a commit that referenced this pull request Jan 17, 2024

[3.9] gh-91133: tempfile.TemporaryDirectory: fix symlink bug in clean…

d54e22a

…up (GH-99930) (GH-112842) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]>

ambv pushed a commit that referenced this pull request Jan 17, 2024

[3.10] gh-91133: tempfile.TemporaryDirectory: fix symlink bug in clea…

8eaeefe

…nup (GH-99930) (GH-112840) (cherry picked from commit 81c16cd) Co-authored-by: Søren Løvborg <[email protected]>

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-91133: tempfile.TemporaryDirectory: fix symlink bug in clean…

762c12e

…up (pythonGH-99930) Co-authored-by: Serhiy Storchaka <[email protected]>

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-91133: tempfile.TemporaryDirectory: fix symlink bug in clean…

0b01b50

…up (pythonGH-99930) Co-authored-by: Serhiy Storchaka <[email protected]>

Uh oh!

Comments

Conversation

kwi-dk commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 1, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for python-cpython-preview ready!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arhadthedev commented Feb 28, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Dec 7, 2023

Uh oh!

miss-islington-app bot commented Dec 7, 2023

Uh oh!

miss-islington-app bot commented Dec 7, 2023

Uh oh!

miss-islington-app bot commented Dec 7, 2023

Uh oh!

miss-islington-app bot commented Dec 7, 2023

Uh oh!

bedevere-app bot commented Dec 7, 2023

Uh oh!

afeblot commented Dec 7, 2023

Uh oh!

bedevere-app bot commented Dec 7, 2023

Uh oh!

bedevere-app bot commented Dec 7, 2023

Uh oh!

bedevere-app bot commented Dec 7, 2023

Uh oh!

vstinner commented Dec 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kwi-dk commented Dec 1, 2022 •

edited

Loading

ghost commented Dec 1, 2022 •

edited by ghost

Loading

netlify bot commented Dec 14, 2022 •

edited

Loading