This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: chdir __exit__ is not safe
Type: behavior Stage: patch review
Components: Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: FFY00, barry, eryksun, ucodery
Priority: normal Keywords: patch

Created on 2021-10-20 22:54 by ucodery, last changed 2022-04-11 14:59 by admin.

Files
File name Uploaded Description Edit
test_chdir.py ucodery, 2021-10-20 22:54 minimal test script
Pull Requests
URL Status Linked Edit
PR 29218 open ucodery, 2021-10-25 20:54
PR 29220 open FFY00, 2021-10-26 00:20
Messages (14)
msg404534 - (view) Author: Jeremy (ucodery) * Date: 2021-10-20 22:54
The way that contextlib.chdir currently restores the old working directory, an exception is raised if the program was already close to or beyond a system's PATH_MAX. The context manager has no issue crafting the path in __enter__ because os.getcwd() can return a path that is longer than PATH_MAX, but when used in __exit__ os.chdir() cannot use a path that long.

I think an __exit__ should be as cautious as possible to not raise as the exception can occur far away from where the context manager was created. Its also doesn't reflect the programmer actually using the context manager incorrectly as they might not have any control or care where the process was started, yet if it happened to already be at a deep path when launched, any use of chdir anywhere would cause exceptions.

I have tested this on macOS 11.13 using APFS but I am sure it would also fail on other macs and Linuxes. I don't know about Windows. Note I originally created this test as a patch to Lib/tests/test_contextlib.py but libregrtest uses os.getcwd() in its runner and that disrupts the traceback and misidentifies the cause of failure. Test file:

```python
import os
import shutil
from contextlib import chdir


def test_long_path():
    # NAME_MAX of 255
    long_filename = "_".join(["testdir"]*32)
    long_path_end = startingwd = os.getcwd()
    try:
        # I thought this would have to be 16, i.e. a path length over 4096, PATH_MAX
        # but seemingly just crossing 1050 is enough to fail
        for _ in range(4):
            os.mkdir(long_filename)
            os.chdir(long_filename)
            long_path_end = os.path.join(long_path_end, long_filename)
        os.mkdir(long_filename)
        long_path_end = os.path.join(long_path_end, long_filename)
        with chdir(long_filename):
            #self.assertEqual(os.getcwd(), long_path_end)
            assert os.getcwd() == long_path_end
            print("passed")
    finally:
        shutil.rmtree(os.path.join(startingwd, long_filename), ignore_errors=True)


test_long_path()
```

And output:
```
$ ./python.exe ./test_chdir.py
passed
Traceback (most recent call last):
  File "/Users/ucodery/git/cpython/./test_chdir.py", line 27, in <module>
    test_long_path()
    ^^^^^^^^^^^^^^^^
  File "/Users/ucodery/git/cpython/./test_chdir.py", line 19, in test_long_path
    with chdir(long_filename):
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ucodery/git/cpython/Lib/contextlib.py", line 781, in __exit__
    os.chdir(self._old_cwd.pop())
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: [Errno 63] File name too long: '/Users/ucodery/git/cpython/testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir/testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir/testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir/testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir'
```
msg404550 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2021-10-21 00:03
Does this mean that CWD could be in a directory that you couldn't chdir() back into?
msg404552 - (view) Author: Jeremy (ucodery) * Date: 2021-10-21 00:15
Yes, precisely. Besides being an unreachable long abs path, it might have been deleted since last visited. I’m working on a few more odd test cases.
msg404557 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2021-10-21 00:25
> Yes, precisely. Besides being an unreachable long abs path, it might have been deleted since last visited. I’m working on a few more odd test cases.

Ah, the deleted case.  Sounds like LBYL wouldn’t work in that case then. :(
msg404568 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-10-21 04:56
> # I thought this would have to be 16, i.e. a path length over 4096, PATH_MAX
> # but seemingly just crossing 1050 is enough to fail

If os.pathconf() and PC_PATH_MAX are supported, the maximum allowed length of an absolute path is os.pathconf('/', os.pathconf_names['PC_PATH_MAX']). I think it also depends on the mounted filesystems that the path traverses.

If os.chdir is in os.supports_fd, the context manager can use dirfd = os.open(os.getcwd(), os.O_RDONLY). Using an fd should also work around the deleted directory case, though POSIX doesn't specify whether fchdir() succeeds in this case. It does in Linux, and the resulting state is the same as deleting the current working directory.

In Windows, SetCurrentDirectoryW() resolves the full path. So the result from os.getcwd() should always work with os.chdir(). The context manager could prevent the original directory from getting deleted by opening it without delete sharing (e.g. via _winapi.CreateFile). Though it may be more reasonable to just let it fail to restore the original working directory.
msg404613 - (view) Author: Jeremy (ucodery) * Date: 2021-10-21 15:58
>If os.chdir is in os.supports_fd, the context manager can use dirfd = os.open(os.getcwd(), os.O_RDONLY). Using an fd should also work around the deleted directory case, though POSIX doesn't specify whether fchdir() succeeds in this case. It does in Linux, and the resulting state is the same as deleting the current working directory.

Yes, I was considering an open fd to guarantee to return to the old pwd as long as it existed. I said as much on the mailing list, but was uncertain if it was possible do deadlock holding on to arbitrary directory handles. If it's possible at all to deadlock, and I think it is, I don't think we can use this; not in a stdlib implementation. The reason for the deadlock is too hidden from the user and debugging it would be difficult. It would be fine for a user implementation where they understood the implications and made other guarantees about their traversals, but we can't be sure everyone using this implementation would read an understand this limitation.

I hadn't considered systems that don't support fd vops. I also hadn't considered crossing mount points and if that could cause any additional error cases. I don't think it can, not that we could correct in user-space and with just using os.chdir().

>In Windows, SetCurrentDirectoryW() resolves the full path. So the result from os.getcwd() should always work with os.chdir(). The context manager could prevent the original directory from getting deleted by opening it without delete sharing (e.g. via _winapi.CreateFile). Though it may be more reasonable to just let it fail to restore the original working directory.

Thanks, I am much less familiar with these APIs. So I believe you are saying the implementation as is will work in all reasonable cases for Windows.
I think attempting to move back to a directory that has been removed should be an error. Especially if we give the same behavior on Linux. Producing a FileNotFoundError gives the user the power to decide if they do in fact want to handle that differently.
msg404718 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-10-22 02:35
Sorry, I seem to have missed your post last month when I scanned over the thread on python-ideas [1]. 

In POSIX, it could try to handle ENAMETOOLONG by walking the path forward with a relative chdir(). This could try to consume as many components as possible in each pass according to os.pathconf(cwd, os.pathconf_names['PC_PATH_MAX']), starting with cwd='/' and subsequently cwd=".". However, the return value is unreliable if the relative path traverses a mount point into a more restrictive filesystem [2]. It's probably good enough to just walk forward one component at a time.

---
[1] https://mail.python.org/archives/list/python-ideas@python.org/thread/C525UVPP3ALGTXDNFL2GFDV23KCHP3RL
[2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/pathconf.html
msg405011 - (view) Author: Filipe Laíns (FFY00) * (Python triager) Date: 2021-10-26 00:15
Alternatively, can't we just os.chdir(self._old_cwd) in __enter__ and preemptively fail? IMO it's probably better to just straight up fail if we can chdir back to the original directory than to have relatively fragile recovery logic.
msg405012 - (view) Author: Filipe Laíns (FFY00) * (Python triager) Date: 2021-10-26 00:16
s/if we can chdir/if we can't chdir/
msg405019 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2021-10-26 02:04
Does a LBYL strategy actually fix the problem?  E.g. what if the directory gets rm'd between __enter__ and __exit__?  Maybe we shouldn't try to be clever at all and just leave it to the user to decide what to do, and how to handle any chdir-back failures?  Keep it simple?
msg405021 - (view) Author: Jeremy (ucodery) * Date: 2021-10-26 03:31
A LBYL won't always raise errors early as you point out. It will give earlier warnings for a lot of cases, but makes contextlib.chdir usable in less places than os.chdir.
Some return paths will always be errors, and some will be technically recoverable but too difficult to detect and or fragile. That's why I think any solution should incorporate the `ignore_errors` flag. Its pretty ugly to wrap a context manager in a try: except: just because you were trying to clean up after whatever you were doing but the cwd changed in unexpected ways, maybe out of your control.
msg405022 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2021-10-26 04:48
> A LBYL won't always raise errors early as you point out. It will give earlier warnings for a lot of cases, but makes contextlib.chdir usable in less places than os.chdir.
> Some return paths will always be errors, and some will be technically recoverable but too difficult to detect and or fragile. That's why I think any solution should incorporate the `ignore_errors` flag. Its pretty ugly to wrap a context manager in a try: except: just because you were trying to clean up after whatever you were doing but the cwd changed in unexpected ways, maybe out of your control.

How common do you expect such errors to be though?  Do you expect them to be more or less common than with os.chdir()?  Do you expect the mitigations to be any different than with a failing os.chdir()?

I’ve certainly written a chdir context manager several times and for the use cases I care about, I’ve never had such a failure, at least not one that wasn’t caused by some other underlying bug, which I was glad wasn’t silenced.
msg405025 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-10-26 05:37
> Alternatively, can't we just os.chdir(self._old_cwd) in __enter__ and preemptively fail? 

If the context manager isn't going to address the long-path case reliably using either a file-descriptor approach or repeated relative chdir() calls, then I think failing early like this is the next best choice.

The previous directory getting deleted is a random environment error, which can be left up to the caller. In POSIX, it might be avoidable using a file-descriptor approach, but POSIX doesn't actually guarantee that fchdir() will succeed if the file descriptor refers to a deleted directory.
msg405052 - (view) Author: Jeremy (ucodery) * Date: 2021-10-26 17:30
> How common do you expect such errors to be though?  Do you expect them to be more or less common than with os.chdir()?  Do you expect the mitigations to be any different than with a failing os.chdir()?

It has come up for me with some frequency. But I'm sure my use case is an outlier, stress testing filesystems and working on backup/restore. The thing about needing to access long paths is that you have to do it with these leaps of <= PATH_MAX until you get close enough to the end. Whether you use relative paths or open fds, you have to get there slowly and then walk back along the same path. This would be greatly simplified by contextlib.chdir if it isn't restricted to absolute paths; otherwise it will remain as much a manual effort as ever.

It also has to do with the scope of any try block. If we leave any exceptions to bubble up to the caller, then any code in the with block is also being guarded. Presumably the caller used chdir because they want to do more os operations in the with block, but they won't be able to sort out if the ENOENT or similar error was from the context manager or their own, perhaps more critical, os operations.


> If the context manager isn't going to address the long-path case reliably using either a file-descriptor approach or repeated relative chdir() calls, then I think failing early like this is the next best choice.

I thought about going down the fd road but as not every platform can chdir to a fd, the relative way back would have to be implemented anyways. It didn't seem worth it to have different platforms behave differently on exiting the context manager.
History
Date User Action Args
2022-04-11 14:59:51adminsetgithub: 89708
2021-10-26 17:30:32ucoderysetmessages: + msg405052
2021-10-26 05:37:29eryksunsetmessages: + msg405025
2021-10-26 04:48:06barrysetmessages: + msg405022
2021-10-26 03:31:26ucoderysetmessages: + msg405021
2021-10-26 02:04:19barrysetmessages: + msg405019
2021-10-26 00:20:59FFY00setpull_requests: + pull_request27483
2021-10-26 00:16:53FFY00setmessages: + msg405012
2021-10-26 00:15:33FFY00setmessages: + msg405011
2021-10-25 20:54:13ucoderysetkeywords: + patch
stage: patch review
pull_requests: + pull_request27481
2021-10-22 11:41:55FFY00setnosy: + FFY00
2021-10-22 02:35:17eryksunsetmessages: + msg404718
2021-10-21 15:58:55ucoderysetmessages: + msg404613
2021-10-21 04:56:17eryksunsetnosy: + eryksun
messages: + msg404568
2021-10-21 00:25:14barrysetmessages: + msg404557
2021-10-21 00:15:09ucoderysetmessages: + msg404552
2021-10-21 00:03:53barrysetnosy: + barry
messages: + msg404550
2021-10-20 22:54:45ucoderycreate