bpo-46785: Fix race condition between `os.stat()` and unlink #31858

itaisteinherz · 2022-03-13T20:16:57Z

https://bugs.python.org/issue46785

…n32_xstat_impl`

the-knights-who-say-ni · 2022-03-13T20:17:02Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@itaisteinherz

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Lib/test/test_os.py

Modules/posixmodule.c

itaisteinherz · 2022-03-18T11:55:22Z

Would be great to have someone take a look at this PR :)

Lib/test/test_os.py

eryksun · 2022-03-19T07:45:46Z

Lib/test/test_os.py

+                proc.terminate()
+
+    @support.requires_subprocess()
+    def test_stat_rmdir_race(self):


I don't think another test of FileNotFoundError is needed. Maybe there could be a test for the ERROR_NOT_READY case due to media removal, but I don't see how to set that up with the tools at hand, and it's so rare that it's not a practical problem. The far more likely case is that CreateFileW() fails with ERROR_NOT_READY, which isn't handled in os.stat().

Regarding the test - why not? I don't like the code duplication, but the test isn't too complicated, and will verify in the future that no regressions occur.

Regarding ERROR_NOT_READY not being handled - I didn't notice that. It seems to me like handling ERROR_NOT_READY and ERROR_BAD_NET_NAME in this PR doesn't make much sense then, and should be done (and tested manually/programmatically) in another PR. (By the way, is there an issue in the issue tracker for this?)

The ERROR_PATH_NOT_FOUND case is basically the same condition as ERROR_FILE_NOT_FOUND. When CreateFileW() is called, the file and its parent directory still exist, but by the time FindFirstFileW() is called, one or both is gone. I can't see anyone ever modifying the os.stat() implementation to remove handling of one error but not the other. These two errors are the far more likely cases, e.g. some other thread is removing the tree with something like shutil.rmtree().

ERROR_PATH_NOT_FOUND would also occur if a removable drive gets removed with freakish coincidental timing. That's basically the same case as ERROR_BAD_NETNAME (share removed) and ERROR_NOT_READY (removable media removed). These are all very unlikely in practice, but they're possible, and they fall into the same general category of the file no longer exists. Returning a PermissionError for ERROR_ACCESS_DENIED (5) or ERROR_SHARING_VIOLATION (32) for a file that no longer exists is pointless. It's hard to test these cases, but that doesn't mean they should be excluded from the PR entirely.

The ERROR_PATH_NOT_FOUND case is basically the same condition as ERROR_FILE_NOT_FOUND.

I don't disagree. I'll remove the test.

It's hard to test these cases, but that doesn't mean they should be excluded from the PR entirely.

I'm fine with including them, but I also wanted to make sure that there's an issue for the more general case you mentioned.

itaisteinherz · 2022-03-27T11:58:33Z

Bump

MaxwellDupre

Ran 316 tests in 6.177s
OK (skipped=47)
Looks ok.

zooba · 2022-05-02T15:20:51Z

Hopefully that resets the broken status. This looks fine to merge once CI finishes.

itaisteinherz · 2022-05-03T05:39:44Z

@zooba Do I need to manually backport this?

miss-islington · 2022-05-07T23:48:33Z

Thanks @itaisteinherz for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington · 2022-05-07T23:48:34Z

Thanks @itaisteinherz for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

zooba · 2022-05-07T23:48:45Z

Let's find out if it needs to be done manually...

miss-islington · 2022-05-07T23:48:58Z

Sorry, @itaisteinherz and @zooba, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 39e6b8ae6a5b49bb23746fdcc354d148ff2d98e3 3.10

miss-islington · 2022-05-07T23:48:59Z

Sorry @itaisteinherz and @zooba, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 39e6b8ae6a5b49bb23746fdcc354d148ff2d98e3 3.9

zooba · 2022-05-07T23:49:16Z

Yeah, looks like it needs to be manual.

…Windows (pythonGH-31858). (cherry picked from commit 39e6b8a) Co-authored-by: Itai Steinherz <itaisteinherz@gmail.com>

bedevere-bot · 2022-05-09T21:13:58Z

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

…indows (pythonGH-31858). (cherry picked from commit 39e6b8a) Co-authored-by: Itai Steinherz <itaisteinherz@gmail.com>

bedevere-bot · 2022-05-09T21:21:38Z

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

…GH-31858) * [3.9] bpo-46785: Fix race condition between os.stat() and unlink on Windows (GH-31858). (cherry picked from commit 39e6b8a) Co-authored-by: Itai Steinherz <itaisteinherz@gmail.com>

…GH-31858) * [3.10] bpo-46785: Fix race condition between os.stat() and unlink on Windows (GH-31858). (cherry picked from commit 39e6b8a) Co-authored-by: Itai Steinherz <itaisteinherz@gmail.com>

…pythonGH-31858) * [3.9] bpo-46785: Fix race condition between os.stat() and unlink on Windows (pythonGH-31858). (cherry picked from commit 39e6b8a) Co-authored-by: Itai Steinherz <itaisteinherz@gmail.com>

itaisteinherz added 3 commits March 13, 2022 19:08

Gracefully handle expected errors returned by FindFirstFileW in `wi…

4b7d59d

…n32_xstat_impl`

Add test for bugfix

3c841f3

Add myself to Misc/ACKS

cfc3f2b

bedevere-bot added the awaiting review label Mar 13, 2022

the-knights-who-say-ni added the CLA not signed label Mar 13, 2022

blurb-it bot and others added 4 commits March 13, 2022 20:35

📜🤖 Added by blurb_it.

cb4896b

Remove resolved TODO

b78e22c

Resolve all TODOs

918e1ba

Document caught error codes

d576c3f

itaisteinherz marked this pull request as ready for review March 13, 2022 21:19

Remove duplicate call to GetLastError()

a8fb3f7

eryksun reviewed Mar 14, 2022

View reviewed changes

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

itaisteinherz commented Mar 14, 2022

View reviewed changes

Modules/posixmodule.c Show resolved Hide resolved

CR improvement

88fb1fe

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Mar 14, 2022

zooba reviewed Mar 18, 2022

View reviewed changes

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

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

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

itaisteinherz added 2 commits March 18, 2022 15:18

CR improvements

5ac6f47

Remove unused variable

b212087

eryksun reviewed Mar 18, 2022

View reviewed changes

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

itaisteinherz added 3 commits March 18, 2022 15:52

Terminate test thread at end of test

7669397

Add test for race between rmdir and stat

1963993

Improve bugfix description

4c0f342

itaisteinherz requested a review from zooba March 18, 2022 18:30

eryksun reviewed Mar 19, 2022

View reviewed changes

itaisteinherz added 2 commits March 19, 2022 16:29

Remove new test

765194c

Merge branch 'main' into bugfix/bpo-46785-os-stat-error-handling

7d8e146

MaxwellDupre approved these changes May 1, 2022

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 1, 2022

zooba closed this May 2, 2022

zooba reopened this May 2, 2022

zooba merged commit 39e6b8a into python:main May 2, 2022

bedevere-bot removed the awaiting core review label May 2, 2022

itaisteinherz deleted the bugfix/bpo-46785-os-stat-error-handling branch May 3, 2022 05:39

zooba added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels May 7, 2022

miss-islington assigned zooba May 7, 2022

bedevere-bot removed the needs backport to 3.10 only security fixes label May 9, 2022

bedevere-bot removed the needs backport to 3.9 only security fixes label May 9, 2022

anntzer mannequin mentioned this pull request Apr 10, 2022

On Windows, os.stat() can fail if called while another process is creating or deleting the file #90941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-46785: Fix race condition between `os.stat()` and unlink #31858

bpo-46785: Fix race condition between `os.stat()` and unlink #31858

itaisteinherz commented Mar 13, 2022 •

edited

Loading

the-knights-who-say-ni commented Mar 13, 2022

itaisteinherz commented Mar 18, 2022

eryksun Mar 19, 2022

itaisteinherz Mar 19, 2022

eryksun Mar 19, 2022 •

edited

Loading

itaisteinherz Mar 19, 2022

itaisteinherz commented Mar 27, 2022

MaxwellDupre left a comment

zooba commented May 2, 2022

itaisteinherz commented May 3, 2022

miss-islington commented May 7, 2022

miss-islington commented May 7, 2022

zooba commented May 7, 2022

miss-islington commented May 7, 2022

miss-islington commented May 7, 2022

zooba commented May 7, 2022

bedevere-bot commented May 9, 2022

bedevere-bot commented May 9, 2022

bpo-46785: Fix race condition between os.stat() and unlink #31858

bpo-46785: Fix race condition between os.stat() and unlink #31858

Conversation

itaisteinherz commented Mar 13, 2022 • edited Loading

the-knights-who-say-ni commented Mar 13, 2022

CLA Missing

itaisteinherz commented Mar 18, 2022

eryksun Mar 19, 2022

Choose a reason for hiding this comment

itaisteinherz Mar 19, 2022

Choose a reason for hiding this comment

eryksun Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

itaisteinherz Mar 19, 2022

Choose a reason for hiding this comment

itaisteinherz commented Mar 27, 2022

MaxwellDupre left a comment

Choose a reason for hiding this comment

zooba commented May 2, 2022

itaisteinherz commented May 3, 2022

miss-islington commented May 7, 2022

miss-islington commented May 7, 2022

zooba commented May 7, 2022

miss-islington commented May 7, 2022

miss-islington commented May 7, 2022

zooba commented May 7, 2022

bedevere-bot commented May 9, 2022

bedevere-bot commented May 9, 2022

bpo-46785: Fix race condition between `os.stat()` and unlink #31858

bpo-46785: Fix race condition between `os.stat()` and unlink #31858

itaisteinherz commented Mar 13, 2022 •

edited

Loading

eryksun Mar 19, 2022 •

edited

Loading