homepage

Patches for getpath.c in Python 2.7 and 3.2a3:
2.7 chunk#2: copy_absolute() would use uninitialized data if getcwd()
failed. The fix is equivalent to what 3.2a3 does.
3.2a3 chunk#2: search_for_exec_prefix() did 'unsigned value >= 0' on the
PyUnicode_AsWideChar() result. (The fix just renames n to k of signed
type, and moves the variables. An outer uninitialized 'size_t n' is in
scope, so renaming the inner n to k leaves 'n=fread()' still a size_t.)
Chunk #1, both versions: Fix an unlikely 'n+k' wraparound bug while I'm
at it. The code has just checked that MAXPATHLEN-n will not wrap.

3.2a3 chunk#2 cannot fail because PyUnicode_AsWideChar() only returns -1 if the first argument is NULL, whereas we just checked that decoded is not NULL. But it would be better to fix the variable type to avoid future bugs.

r86399 fixes the code checking PyUnicode_AsWideChar() failure.
The following change is useless, it cannot overflow:
- if (n + k > MAXPATHLEN)
+ if (k > MAXPATHLEN - n)
 k = MAXPATHLEN - n;
n and k maximum values are MAXPATHLEN (and the maximum value of MAXPATHLEN is 4096), whereas n and k type maximum values are at least 2^31.

STINNER Victor writes:
> The following change is useless, it cannot overflow:
> - if (n + k > MAXPATHLEN)
> + if (k > MAXPATHLEN - n)
> k = MAXPATHLEN - n;
> 
> n and k maximum values are MAXPATHLEN (and the maximum value of
> MAXPATHLEN is 4096), (...)
OK. I could only tell that for n, not for k. But even so it would
certainly be an unlikely overflow.

Reading msg120918 and msg120940 it looks as if work has been completed so this can be closed as fixed.

> Reading msg120918 and msg120940 it looks as if work has been completed so this can be closed as fixed.
Attached patch contains more fixes.

I reviewed getpath.diff:
diff -pru Python-2.7/Modules/getpath.c Python-2.7/Modules/getpath.c
--- Python-2.7/Modules/getpath.c
+++ Python-2.7/Modules/getpath.c
@@ -218,7 +218,7 @@ joinpath(char *buffer, char *stuff)
 if (n > MAXPATHLEN)
 Py_FatalError("buffer overflow in getpath.c's joinpath()");
 k = strlen(stuff);
- if (n + k > MAXPATHLEN)
+ if (k > MAXPATHLEN - n)
 k = MAXPATHLEN - n;
 strncpy(buffer+n, stuff, k);
 buffer[n+k] = '0円';
As I explained in a previous comment, this change is useless: "n+k" cannot overflow.
@@ -229,10 +229,9 @@ joinpath(char *buffer, char *stuff)
 static void
 copy_absolute(char *path, char *p)
 {
- if (p[0] == SEP)
+ if (p[0] == SEP || getcwd(path, MAXPATHLEN) == NULL)
 strcpy(path, p);
 else {
- getcwd(path, MAXPATHLEN);
 if (p[0] == '.' && p[1] == SEP)
 p += 2;
 joinpath(path, p);
The code changed in the meanwhile (in the master branch, in 3.7 maybe, I don't recall), getcwd() error is now checked in copy_absolute():
 if (!_Py_wgetcwd(path, pathlen)) {
 /* unable to get the current directory */
 wcscpy(path, p);
 return;
 }
diff -pru Python-3.2a3/Modules/getpath.c Python-3.2a3/Modules/getpath.c
--- Python-3.2a3/Modules/getpath.c
+++ Python-3.2a3/Modules/getpath.c
@@ -351,18 +351,18 @@ search_for_exec_prefix(wchar_t *argv0_pa
 else {
 char buf[MAXPATHLEN+1];
 PyObject *decoded;
- wchar_t rel_builddir_path[MAXPATHLEN+1];
- size_t n;
 n = fread(buf, 1, MAXPATHLEN, f);
 buf[n] = '0円';
 fclose(f);
 decoded = PyUnicode_DecodeUTF8(buf, n, "surrogateescape");
 if (decoded != NULL) {
- n = PyUnicode_AsWideChar((PyUnicodeObject*)decoded,
+ wchar_t rel_builddir_path[MAXPATHLEN+1];
+ Py_ssize_t k;
+ k = PyUnicode_AsWideChar((PyUnicodeObject*)decoded,
 rel_builddir_path, MAXPATHLEN);
 Py_DECREF(decoded);
- if (n >= 0) {
- rel_builddir_path[n] = L'0円';
+ if (k >= 0) {
+ rel_builddir_path[k] = L'0円';
 wcscpy(exec_prefix, argv0_path);
 joinpath(exec_prefix, rel_builddir_path);
 return -1;
Again, the code changed in the meanwhile:
 wchar_t *rel_builddir_path;
 ...
 rel_builddir_path = _Py_DecodeUTF8_surrogateescape(buf, n);
 if (rel_builddir_path) {
There is a new _Py_DecodeUTF8_surrogateescape() function which returns directly wchar_t* strings.
I close the issue.
--
Modules/getpath.c should be rewritten without global MAXPATHLEN limit. I rewrote the code to avoid global variables and pass buffer lengths in some functions. It's better but not perfect yet :-))