Use Py_CLEAR instead of Py_DECREF to also set the variable to NULL

[ret2libc]

These files contain loops that convert system data into python objects
and during the process they create objects and dereference their
refcounts after they have been added to the resulting list.

However, in case of errors during the creation of those python objects,
the refcount to previously allocated objects is dropped again with
Py_XDECREF, which should be a no-op in case the paramater is NULL. Even
so, in most of these loops the variables pointing to the objects are
never set to NULL, even after Py_DECREF is called at the end of the loop
iteration. This means, after the first iteration, if an error occurs
those python objects will get their refcount dropped two times,
resulting in a possible double-free.

In any case, making functions like PyUnicode_DecodeFSDefault fail does not seem like an easy thing, as after playing a bit with it, it seems like it fails only when there's not enough memory (or other very particular conditions apply).

[giampaolo]

in most of these loops the variables pointing to the objects are never set to NULL

That's implicit:

    PyObject *py_retlist;

    if (py_retlist == NULL)
        printf("is null\n");
    else
        printf("is not null\n");

...prints "is null". For the Py_[X]DECREF dance I took inspiration from cPython source code. E.g. try to grep for PyList_Append and you'll see the same approach is being used.

[giampaolo]

Ah, I understand what you mean. Py_DECREF does not set the object to NULL, and that's a problem in while/for loops (and only in that case). I think you're right.

[ret2libc]

in most of these loops the variables pointing to the objects are never set to NULL

That's implicit:

    PyObject *py_retlist;

    if (py_retlist == NULL)
        printf("is null\n");
    else
        printf("is not null\n");

...prints "is null". For the Py_[X]DECREF dance I took inspiration from cPython source code. E.g. try to grep for PyList_Append and you'll see the same approach is being used.

Also, it is not implicit that a variable on the stack is set to NULL. It is an error to assume so in general. I am not sure about python extensions in C, but I do think it is the same, so you should always initialize your local variables before using them. They don't get implicitly set to NULL.

[giampaolo]

They don't get implicitly set to NULL.

Does it depend from the compiler? On Linux + gcc 7.4.0 they do.

[ret2libc]

They don't get implicitly set to NULL.

Does it depend from the compiler? On Linux + gcc 7.4.0 they do.

It may depend on the compiler, but AFAIK gcc does not automatically initialize local variables. The fact that in some tests you see 0 as value does not mean they will always have. Usually uninitialized local variables just contain the garbage value that was left on the stack. For what I know, the C99 standard says that local variables that are not explicitly initialized may contain any value, so you can't rely on those variable contain 0.

[ret2libc]

Apparently CVE-2019-18874 was assigned by MITRE to this issue.

[giampaolo]

Yep. The theoretical risk is being able to create a disk/partition with an invalid character and make PyUnicode_DecodeFSDefault fail. The double free will result in a segfault. CVE creation was coordinated by Tidelift.

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-18874

[giampaolo]

Thanks a lot for the fix BTW.

[VojtechBartos]

Thanks for the fix!

@giampaolo is there any plan for releasing a patch version to get this fix out? CVE has been assigned https://snyk.io/vuln/SNYK-PYTHON-PSUTIL-483082

[SteveJae]

Quick follow-up: Is there a timeline for a new release including the patch? Thanks a lot in advance.