[issue45545] chdir __exit__ is not safe

[Jeremy]

Jeremy  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Eryk Sun]

Eryk Sun  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Barry A. Warsaw]

Barry A. Warsaw  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Jeremy]

Jeremy  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Barry A. Warsaw]

Barry A. Warsaw  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Filipe Laíns]

Change by Filipe Laíns :


--
pull_requests: +27483
pull_request: https://github.com/python/cpython/pull/29220

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Filipe Laíns]

Filipe Laíns  added the comment:

s/if we can chdir/if we can't chdir/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Filipe Laíns]

Filipe Laíns  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Jeremy]

Change by Jeremy :


--
keywords: +patch
pull_requests: +27481
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/29218

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Filipe Laíns]

Change by Filipe Laíns :


--
nosy: +FFY00

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Eryk Sun]

Eryk Sun  added the comment:

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-id...@python.org/thread/C525UVPP3ALGTXDNFL2GFDV23KCHP3RL
[2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/pathconf.html

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Jeremy]

Jeremy  added the comment:

>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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Eryk Sun]

Eryk Sun  added the comment:

> # 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.

--
nosy: +eryksun

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Barry A. Warsaw]

Barry A. Warsaw  added the comment:

> 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. :(

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Jeremy]

Jeremy  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Barry A. Warsaw]

Barry A. Warsaw  added the comment:

Does this mean that CWD could be in a directory that you couldn't chdir() back 
into?

--
nosy: +barry

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue45545] chdir __exit__ is not safe

[Jeremy]

New submission from Jeremy :

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 
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_te
 
stdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir_testdir'
```

--
files: test_chdir.py
messages: 404534
nosy: ucodery
priority: normal
severity: normal
status: open
title: chdir __exit__ is not safe
type: behavior
versions: Python 3.11
Added file: https://bugs.python.org/file50377/test_chdir.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com