gh-157579: Fix race condition in the cleanup of tempfile.TemporaryDirectory

[StanFromIreland]

• Issue: Race condition in the cleanup of tempfile.TemporaryDirectory #157579

[read-the-docs-community]

Documentation build overview

📚 cpython-previews | 🛠️ Build #34621385 | 📁 Comparing de46e06 against main (82952e3)

  🔍 Preview build  

6 files changed · ± 6 modified

± Modified

• c-api/bytes.html
• library/importlib.html
• library/tempfile.html
• library/unicodedata.html
• whatsnew/3.16.html
• whatsnew/changelog.html

[StanFromIreland]

I'm not sure why Emscripten failed in this run on test_cleanup_with_symlink_race:

 ======================================================================
FAIL: test_cleanup_with_symlink_race (test.test_tempfile.TestTemporaryDirectory.test_cleanup_with_symlink_race)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/test/test_tempfile.py", line 1907, in test_cleanup_with_symlink_race
    self.assertTrue(os.path.exists(target_file))
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: False is not true

The test swaps a directory for a symlink to the outside directory mid-recovery, however the unlink followed the symlink and deleted it. This is odd because the unlink was passed a dir_fd for the directory that got swapped out. However, it doesn’t hit our skip (so the platform meets the requirements, which means os.unlink is indeed in os.supports_dir_fd). CC @hoodmane any idea what's going on here?

[hoodmane]

Will take a look.

[encukou]

Meanwhile let's start buildbots for all the other architectures/configurations

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @encukou for commit a837624 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F157580%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[StanFromIreland]

And it seems all the buildbots older with masOS <26 (and iOS) are failing on test_cleanup_with_symlink_race with:

======================================================================
ERROR: test_cleanup_with_symlink_race (test.test_tempfile.TestTemporaryDirectory.test_cleanup_with_symlink_race)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/pull_request.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_tempfile.py", line 1904, in test_cleanup_with_symlink_race
    os.chmod(dir1, 0o700)
    ~~~~~~~~^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/test_python_lxxiashb/3w36qc28/dir1'

We don't know if the test passes or not, as we don't reach the final assertions. I'm also not quite sure why this happens, and I don't have access to a machine to debug. It's possible that the hook's rename of the 0o500 directory fails? Any ideas Petr?

[encukou]

Funny story

I can reproduce! .... on an ancient Intel MacBook that bought it in this state to take macOS screenshots for tutorials. It lasts ~10 min on battery and has a broken screen. It also has only has 1 USB port, so I can charge it, or connect a monitor/LAN, but not both at once. (This was never a problem for screenshots...)

So, work is slow. But, in the end, after several tries, I did manage to find a compatible dongle.

Turns out that on old macOS, you can't rename a directory if you don't have write permissions on that directory. (On Linux and -- apparently -- new macs, you need write on the containing directory; that has the inode you're manipulating.)

---

The rmtree will:

• open d1 & start scandir
• remove test0.txt from d1 via the monkeypatched hook; this succeeds
• open dir1 & start scandir
• remove file1 from dir1 via the monkeypatched hook
  • remove file1 from dir1; this fails with PermissionError
  • rename dir1 to dir1_moved; this also fails due to PermissionError (!)
• call onexc with hook as func, file1 as path, dir1's fd as dir_fd
  • call chflags on dir1
  • call chmod on dir1's fd
  • call chflags on file1
  • call chmod on file1 via dir1's fd
  • remove file1 (via the hook; this succeeds)
• remove dir1 via d1's fd
• rmdir d1 via path

Back in the test, in the finally:

• islink is False as dir1 doesn't exist
• chmod fails as dir1 doesn't exist

[StanFromIreland]

Funny story

😆 Oh no, sorry for the trouble, but thank you!

Turns out that on old macOS, you can't rename a directory if you don't have write permissions on that directory.

Aargh, that's interesting. I applied your suggestion, but I think it's better to make it writeable in the hook instead so that we rename properly.

[StanFromIreland]

!buildbot macOS|iOS

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @StanFromIreland for commit de46e06 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F157580%2Fmerge

The command will test the builders whose names match following regular expression: macOS|iOS

The builders matched are:

• ARM64 MacOS M1 NoGIL PR
• x86-64 macOS PR
• x86-64 MacOS Intel NoGIL PR
• x86-64 MacOS Intel ASAN NoGIL PR
• ARM64 MacOS M1 Refleaks NoGIL PR
• ARM64 macOS PR
• iOS ARM64 Simulator PR