bpo-32904: Fix a potential crash in os.chdir() and os.getcwd() on Windows

[izbyshev]

win32_wchdir() retries GetCurrentDirectory() with a larger buffer if necessary, but doesn't check whether the new buffer is large enough. Another thread could change the current directory in meanwhile, so the buffer could turn out to be still not large enough, left in an uninitialized state and passed to SetEnvironmentVariable() afterwards.

A similar issue occurs in posix_getcwd().

https://bugs.python.org/issue32904

[github-actions[bot]]

This PR is stale because it has been open for 30 days with no activity.

[izbyshev]

Thank you for reviewing, @eryksun. I'll get back to this PR later this week and address your suggestions.

[izbyshev]

There is also a thing that I don't understand about win32_wchdir() that is not directly related to this PR: why set the per-drive current directory manually instead of using MSVCRT's _wchdir()? It was added in 477c8d5e70240744d24631b18341ad892c8a8e1c in 2006 and had a comment at that time:

/* The Unicode version differs from the ANSI version
   since the current directory might exceed MAX_PATH characters */

But it don't think it's true that SetCurrentDirectoryW() can exceed the MAX_PATH limit even now (regardless of whether long paths are enabled). So it's unclear what is achieved here, unless somebody expects that this limitation will be lifted some day.

[eryksun]

But it don't think it's true that SetCurrentDirectoryW() can exceed the MAX_PATH limit even now (regardless of whether long paths are enabled).

Back then it couldn't, but long-path support in Windows 10+ does allow the current working directory to be a long path. A child process, however, can't inherit a long path as the current working directory. CreateProcessW() fails in this case. For example:

>>> os.chdir(r'C:/Temp/longpath/' + 'a'*255)
>>> try: subprocess.call('python')
... except OSError as e: print(e)
...
[WinError 87] The parameter is incorrect

Note that only drive and regular UNC paths are supported as the working directory. Device paths such as "\\?\" extended paths are not documented as supported, and the API is actually buggy in this case. It wasn't an issue before because the static buffer in the PEB (process environment block) was limited to MAX_PATH characters anyway.

why set the per-drive current directory manually instead of using MSVCRT's _wchdir()?

chdir(), rename(), rmdir(), and remove() were implemented directly back in Python 2.5 in order to get a Windows error code. That was probably a misunderstanding. Most CRT I/O functions that set errno from a Windows error code also set _doserrno.

We should use _doserrno in io.FileIO. Having the original Windows error code from the underlying CreateFileW() call can be helpful when open() fails. Usually several Windows error codes are mapped to a single errno value such as ENOENT or EACCES.

We could also switch back to calling _wchdir() nowadays and get the Windows error from _doserrno.

[izbyshev]

Thank you for clarifying the situation. With regard to long paths and SetCurrentDirectoryW(), I was aware about issues with \\?\ not extending the MAX_PATH limit and the static buffer in PEB, and then incorrectly assumed that long path awareness in Windows 10 doesn't help. Now I've also found your older comment with more clarifications in another issue.

At least now it's clear to me that we do need to retry GetCurrentDirectoryW() with larger buffers if the MAX_PATH-sized buffer was not enough.