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

izbyshev · 2018-02-22T01:03:59Z

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

Modules/posixmodule.c

eryksun · 2018-02-22T04:08:28Z

Modules/posixmodule.c

-            PyMem_RawFree(wbuf2);
-        return resobj;
-    }
+    if (!use_bytes)


os.getcwdb is also incorrect. It shouldn't call the CRT's ANSI function. It needs to call win32_wgetcwd and convert the result to the file-system encoding, which defaults to UTF-8. This was apparently overlooked when implementing PEP 529.

Indeed. According to PEP 529, os.getcwdb should have also been undeprecated. I'll file an issue for that in a while.

github-actions · 2022-08-16T00:14:13Z

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

eryksun · 2022-08-16T05:58:30Z

Modules/posixmodule.c

@@ -1508,36 +1541,28 @@ posix_fildes_fd(int fd, int (*func)(int))
 static BOOL __stdcall
 win32_wchdir(LPCWSTR path)
 {
-    wchar_t path_buf[MAX_PATH], *new_path = path_buf;
-    int result;
+    wchar_t path_buf[MAXPATHLEN], *new_path;


MAXPATHLEN was changed to 256 in Windows. That's a strange value considering MAX_PATH is 260 and buffers sometimes need to be MAX_PATH + 1 even with legacy code.

Anyway, I don't think performance is critical for getcwd() and chdir(). It would be simpler to merge win32_wgetcwd_buf() into win32_wgetcwd(), and change the definition to static wchar_t *win32_wgetcwd(LPDWORD plength). The caller would have to free the buffer.

MAXPATHLEN was changed to 256 in Windows. That's a strange value considering MAX_PATH is 260 and buffers sometimes need to be MAX_PATH + 1 even with legacy code.

MAXPATHLEN is indeed strange, I even opened an issue about it: #88822.

I believe I changed it purely for consistency with other code in posixmodule.c, but this might be the case of foolish consistency. I've now switched to MAX_PATH instead.

eryksun · 2022-08-16T06:05:15Z

Modules/posixmodule.c

+static PyObject *
+win32_wgetcwd(void)
+{
+    wchar_t wbuf[MAXPATHLEN], *wbuf2;
+    PyObject *resobj;
+
+    Py_BEGIN_ALLOW_THREADS
+    wbuf2 = win32_wgetcwd_buf(wbuf, Py_ARRAY_LENGTH(wbuf));
+    Py_END_ALLOW_THREADS
+
+    if (!wbuf2) {
+        return PyErr_SetFromWindowsErr(0);
+    }
+    resobj = PyUnicode_FromWideChar(wbuf2, wcslen(wbuf2));
+    if (wbuf2 != wbuf)
+        PyMem_RawFree(wbuf2);
+    return resobj;
+}


Here's the suggested change to make win32_wgetcwd() a function that doesn't hold the GIL and always allocates the buffer.

Suggested change

static PyObject *

win32_wgetcwd(void)

{

wchar_t wbuf[MAXPATHLEN], *wbuf2;

PyObject *resobj;

Py_BEGIN_ALLOW_THREADS

wbuf2 = win32_wgetcwd_buf(wbuf, Py_ARRAY_LENGTH(wbuf));

Py_END_ALLOW_THREADS

if (!wbuf2) {

return PyErr_SetFromWindowsErr(0);

}

resobj = PyUnicode_FromWideChar(wbuf2, wcslen(wbuf2));

if (wbuf2 != wbuf)

PyMem_RawFree(wbuf2);

return resobj;

}

static wchar_t *

win32_wgetcwd(wchar_t *buf, size_t size)

{

wchar_t *local_buf = buf;

DWORD local_size = (DWORD)size;

if (size > DWORD_MAX) {

SetLastError(ERROR_INVALID_PARAMETER);

errno = EINVAL;

return NULL;

}

// If buf is NULL, allocate a buffer with the given size. If size is 0,

// the result of GetCurrentDirectoryW() wil be used.

if (buf == NULL && size > 0) {

local_buf = PyMem_RawMalloc(dwsize * sizeof(wchar_t));

if (local_buf == NULL) {

SetLastError(ERROR_NOT_ENOUGH_MEMORY);

errno = ENOMEM;

return NULL;

}

}

while (1) {

wchar_t *temp;

DWORD result = GetCurrentDirectoryW(local_size, local_buf);

if (!result) {

// Set errno for the sake of completeness, but the result is never

// 0 in practice. The call could fail due to an OS exception such

// as an access violation, but that will just crash the process if

// there's no exception handler.

errno = winerror_to_errno(GetLastError());

goto fail;

}

// Note that the null terminator is not counted on success.

if (result < local_size) {

break;

}

// Reallocate only if buf is NULL and size is 0.

if (buf != NULL || size != 0) {

SetLastError(ERROR_INSUFFICIENT_BUFFER);

errno = ERANGE;

goto fail;

}

temp = PyMem_RawRealloc(local_buf, result * sizeof(wchar_t));

if (!temp) {

SetLastError(ERROR_NOT_ENOUGH_MEMORY);

errno = ENOMEM;

goto fail;

}

local_buf = temp;

local_size = result;

}

return local_buf;

fail:

if (buf == NULL && local_buff != NULL) {

PyMem_RawFree(local_buf);

}

return NULL;

}

I've updated the suggestion to implement static wchar_t *win32_wgetcwd(wchar_t *buf, size_t size) more faithfully to POSIX getcwd(). Like glibc, passing buf as NULL supports allocating the buffer. The caller is responsible for freeing an allocated buffer.

I agree with the idea of inlining win32_wgetcwd() back into posix_getcwd(). In 4 years since my original PR handling of use_bytes was changed, and now it seems to make sense to have conversion from the native path type to Python's unicode/bytes in the same place for both Windows and Unix.

However, I can't see the value of introducing a pretty generic POSIX-like win32_wgetcwd() as you suggest. We need to get the current directory in just two places, and I don't see why our needs would change in the future.

I've force-pushed a new commit, which is mostly just a rebase of the original PR + inlining of win32_wgetcwd() and renaming of win32_wgetcwd_buf() to win32_wgetcwd(). ISTM that win32_wgetcwd() is pretty simple. Having the caller to compare the returned buffer to the passed one is surely a wrinkle, but I don't think it matters, and overall I don't see why using your generic win32_wgetcwd() would be much better. But if there is some rationale that I'm missing, I'll be happy to revisit this.

But if there is some rationale that I'm missing, I'll be happy to revisit this.

There's no deep rationale. I just figured I may as well copy the Linux behavior. POSIX getcwd() only says that the behavior is "unspecified" when buf is NULL. Your code implements part of the extended behavior, given buf is NULL and buf_size is 0. However, GetCurrentDirectoryW() assumes that nBufferLength accurately represents the size of lpBuffer. If this size is big enough to hold the path of the current working directory, but lpBuffer is NULL, GetCurrentDirectoryW() will try to write to a NULL pointer and crash on an access violation. This can't happen in how we're currently using the function, but I felt it wouldn't hurt to round out the implementation. The Linux behavior is to allocate a new buffer in this case. Another option is to simply fail if buf_size is greater than 0 when buf is NULL.

I see. I've intended win32_getcwd() purely as a helper for the two cases we have, not as a generic function (and one of those cases shouldn't probably exist at all). I generally don't like to abstract over a small number of use cases. It works for buf is NULL and buf_size is 0 purely by accident. I can add an assert() or a failure case for buf == NULL if you like.

I see how it could be tempting to add a more flexible version, but I think that would be justified if we needed to implement Py_getcwd() for Python/fileutils.c, but not here.

eryksun · 2022-08-16T06:06:39Z

Modules/posixmodule.c

+    if (!use_bytes)
+        return win32_wgetcwd();


With win32_wgetcwd() implemented as a lower-level function, more of the work remains in posix_getcwd(), but it's still relatively simple:

Suggested change

if (!use_bytes)

return win32_wgetcwd();

PyObject *obj

wchar_t *buf;

Py_BEGIN_ALLOW_THREADS

buf = win32_wgetcwd(NULL, 0);

Py_END_ALLOW_THREADS

if (!buf) {

PyErr_NoMemory();

return NULL;

}

obj = PyUnicode_FromWideChar(buf, -1);

PyMem_RawFree(buf);

if (use_bytes && obj) {

Py_SETREF(obj, PyUnicode_EncodeFSDefault(obj));

}

return obj;

izbyshev · 2022-08-17T14:39:36Z

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

…Windows Both functions retry GetCurrentDirectoryW() if the initial buffer is insufficient, but don't check whether the newly allocated buffer is sufficient after the second call. This might not be the case if another thread changes the current directory concurrently, and if so, the functions will proceed to use uninitialized memory. Fix this by retrying GetCurrentDirectoryW() in a loop with larger buffers until it succeeds or we run out of memory. Co-authored-by: Eryk Sun <eryksun@gmail.com>

Modules/posixmodule.c

izbyshev · 2022-08-21T21:09:24Z

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 477c8d5 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 · 2022-08-21T22:42:48Z

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 · 2022-08-22T08:45:10Z

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.

github-actions · 2024-09-21T00:09:21Z

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

asrafulimmd-debug · 2025-08-16T03:16:29Z

gh pr checkout 5802

asrafulimmd-debug · 2025-08-16T03:16:45Z

21-23,25,45,53,80,110,111,113,135,139,143,199,256,443,445,554,587,912,993,995,1720,1723,3306,3389,5900,8080,8888,10257

the-knights-who-say-ni added the CLA signed label Feb 22, 2018

bedevere-bot added the awaiting review label Feb 22, 2018

izbyshev changed the title ~~bpo-32904: Fix a potential crash in os.chdir() on Windows~~ bpo-32904: Fix a potential crash in os.chdir() and os.getcwd() on Windows Feb 22, 2018

eryksun reviewed Feb 22, 2018

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

eryksun reviewed Feb 22, 2018

View reviewed changes

izbyshev force-pushed the bpo-32904 branch from a1e4331 to d572fb3 Compare March 1, 2018 14:15

csabella requested review from zooba and zware October 20, 2019 14:32

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 16, 2022

eryksun reviewed Aug 16, 2022

View reviewed changes

eryksun added OS-windows extension-modules C modules in the Modules dir 3.11 only security fixes 3.10 only security fixes 3.12 only security fixes labels Aug 16, 2022

github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 17, 2022

izbyshev force-pushed the bpo-32904 branch from d572fb3 to 1844ff3 Compare August 21, 2022 19:56

eryksun reviewed Aug 21, 2022

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

eryksun reviewed Aug 21, 2022

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

Add braces per PEP 7

d8c9b7d

eryksun reviewed Aug 21, 2022

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

ERROR_OUTOFMEMORY -> ERROR_NOT_ENOUGH_MEMORY

3b91207

eryksun added the awaiting core review label Aug 22, 2022

arhadthedev added type-security A security issue and removed extension-modules C modules in the Modules dir type-security A security issue labels May 5, 2023

github-actions bot added the stale Stale PR or inactive for long period of time. label Sep 21, 2024

This comment has been minimized.

Sign in to view

asrafulimmd-debug mentioned this pull request Aug 16, 2025

spam #137839

Closed

github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 16, 2025

-static PyObject *
-win32_wgetcwd(void)
-{
-    wchar_t wbuf[MAXPATHLEN], *wbuf2;
-    PyObject *resobj;
-    Py_BEGIN_ALLOW_THREADS
-    wbuf2 = win32_wgetcwd_buf(wbuf, Py_ARRAY_LENGTH(wbuf));
-    Py_END_ALLOW_THREADS
-    if (!wbuf2) {
-        return PyErr_SetFromWindowsErr(0);
-    }
-    resobj = PyUnicode_FromWideChar(wbuf2, wcslen(wbuf2));
-    if (wbuf2 != wbuf)
-        PyMem_RawFree(wbuf2);
-    return resobj;
-}
+static wchar_t *
+win32_wgetcwd(wchar_t *buf, size_t size)
+{
+    wchar_t *local_buf = buf;
+    DWORD local_size = (DWORD)size;
+    if (size > DWORD_MAX) {
+        SetLastError(ERROR_INVALID_PARAMETER);
+        errno = EINVAL;
+        return NULL;
+    }
+    // If buf is NULL, allocate a buffer with the given size. If size is 0,
+    // the result of GetCurrentDirectoryW() wil be used.
+    if (buf == NULL && size > 0) {
+        local_buf = PyMem_RawMalloc(dwsize * sizeof(wchar_t));
+        if (local_buf == NULL) {
+            SetLastError(ERROR_NOT_ENOUGH_MEMORY);
+            errno = ENOMEM;
+            return NULL;
+        }
+    }
+    while (1) {
+        wchar_t *temp;
+        DWORD result = GetCurrentDirectoryW(local_size, local_buf);
+        if (!result) {
+            // Set errno for the sake of completeness, but the result is never
+            // 0 in practice. The call could fail due to an OS exception such
+            // as an access violation, but that will just crash the process if
+            // there's no exception handler.
+            errno = winerror_to_errno(GetLastError());
+            goto fail;
+        }
+        // Note that the null terminator is not counted on success.
+        if (result < local_size) {
+            break;
+        }
+        // Reallocate only if buf is NULL and size is 0.
+        if (buf != NULL || size != 0) {
+            SetLastError(ERROR_INSUFFICIENT_BUFFER);
+            errno = ERANGE;
+            goto fail;
+        }
+        temp = PyMem_RawRealloc(local_buf, result * sizeof(wchar_t));
+        if (!temp) {
+            SetLastError(ERROR_NOT_ENOUGH_MEMORY);
+            errno = ENOMEM;
+            goto fail;
+        }
+        local_buf = temp;
+        local_size = result;
+    }
+    return local_buf;
+fail:
+    if (buf == NULL && local_buff != NULL) {
+        PyMem_RawFree(local_buf);
+    }
+    return NULL;
+}

-    if (!use_bytes)
-        return win32_wgetcwd();
+    PyObject *obj
+    wchar_t *buf;
+    Py_BEGIN_ALLOW_THREADS
+    buf = win32_wgetcwd(NULL, 0);
+    Py_END_ALLOW_THREADS
+    if (!buf) {
+        PyErr_NoMemory();
+        return NULL;
+    }
+    obj = PyUnicode_FromWideChar(buf, -1);
+    PyMem_RawFree(buf);
+    if (use_bytes && obj) {
+        Py_SETREF(obj, PyUnicode_EncodeFSDefault(obj));
+    }
+    return obj;

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

izbyshev commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

eryksun Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

izbyshev Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 16, 2022

Uh oh!

eryksun Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

izbyshev Aug 21, 2022

Choose a reason for hiding this comment

Uh oh!

eryksun Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eryksun Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izbyshev Aug 21, 2022

Choose a reason for hiding this comment

Uh oh!

eryksun Aug 21, 2022

Choose a reason for hiding this comment

Uh oh!

izbyshev Aug 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eryksun Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izbyshev commented Aug 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

izbyshev commented Aug 21, 2022

Uh oh!

eryksun commented Aug 21, 2022

Uh oh!

izbyshev commented Aug 22, 2022

Uh oh!

github-actions bot commented Sep 21, 2024

Uh oh!

This comment has been minimized.

asrafulimmd-debug commented Aug 16, 2025

Uh oh!

asrafulimmd-debug commented Aug 16, 2025

Uh oh!

Uh oh!

izbyshev commented Feb 22, 2018 •

edited

Loading

eryksun Aug 16, 2022 •

edited

Loading

eryksun Aug 17, 2022 •

edited

Loading

izbyshev Aug 21, 2022 •

edited

Loading

eryksun Aug 16, 2022 •

edited

Loading