Modules/getpath.c bugs

[hfuru]

BPO	10308
Nosy	@vstinner
Files	getpath.diff: getpath.c patches

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-02-25.10:10:23.857>
created_at = <Date 2010-11-04.11:27:41.845>
labels = ['type-bug']
title = 'Modules/getpath.c bugs'
updated_at = <Date 2019-02-25.10:10:23.857>
user = 'https://bugs.python.org/hfuru'

bugs.python.org fields:

activity = <Date 2019-02-25.10:10:23.857>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2019-02-25.10:10:23.857>
closer = 'vstinner'
components = []
creation = <Date 2010-11-04.11:27:41.845>
creator = 'hfuru'
dependencies = []
files = ['19486']
hgrepos = []
issue_num = 10308
keywords = ['patch']
message_count = 7.0
messages = ['120390', '120711', '120918', '120940', '241632', '241688', '336507']
nosy_count = 2.0
nosy_names = ['hfuru', 'vstinner']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue10308'
versions = ['Python 3.1', 'Python 2.7', 'Python 3.2']

[hfuru]

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.

[vstinner]

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.

[vstinner]

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.

[hfuru]

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.

[BreamoreBoy]

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

[vstinner]

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

Attached patch contains more fixes.

[vstinner]

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 :-))