#1127 Fix reference counting bug in open_files impl on OSX

[jakub-bacic]

I refactored the code using the following template:

static PyObject *
example_function(PyObject *self, PyObject *args) {
    //1 declare return variable called py_ret in the first line
    PyObject *py_ret = NULL;
    
    //2 declare other python variables (with prefix py_) for easier management of reference counting
    PyObject *py_tuple = NULL;
    PyObject *py_path = NULL;

    //3 declare C variables for easier mapping malloc/free
    struct proc_fdinfo *fds_pointer = NULL;

    //4 other variables here

    //5 main code - in case of error, goto except block
    py_ret = PyList_New(0);
    if (!py_ret)
        goto except;

    // ...

    //6 finally
    goto finally;

except:
    Py_XDECREF(py_ret);
    py_ret = NULL;

finally:
    // call Py_XDECREF on every variable declared in section 2
    Py_XDECREF(py_tuple);
    Py_XDECREF(py_path);

    // call free on every variable declared in section 3 (calling free on NULL is no-op)
    free(fds_pointer);

    return py_ret;
}

Additionally, variables reused in a loop should be properly DECREFed and assigned NULL at the end of the loop (so the presented template works properly in case of flow interruption due to the error).

[giampaolo]

It seems to me like you're changing too much for no good reason. There's no need to redefine variable names, rename "error" goto to "except", introduce "finally" goto etc.
AFAICT the only change which is needed is this one:

              Py_DECREF(py_tuple);
 +            py_tuple = NULL;
              Py_DECREF(py_path);
 +            py_path = NULL;

Am I right?

[jakub-bacic]

Indeed, the minimal change needed to fix this particular issue will look like this.

I introduced more changes only to suggest a cleaner way of dealing with such C code. I assume you want to minimize the change scope as much as possible?

Actually, the smallest fix would be to add:

    for (i = 0; i < iterations; i++) {
        py_tuple = NULL;
+       py_path = NULL;
        fdp_pointer = &fds_pointer[i];

but IMHO it's better to assign NULL right after DECREFing - it might get messy if something happens after the last iteration (talking in general because in this case it doesn't make a difference).

[giampaolo]

Correct, I prefer to minimize the change. Assigning NULL after DECREF is fine.

[jakub-bacic]

Example script to test (it should result in segfault - must be run on Mac OS High Sierra):

import psutil

def test_open_files():
  for proc in psutil.process_iter():
    try:
      proc.open_files()
    except psutil.Error:
      pass

test_open_files()