Re: Fix memleaks and error handling in jsonb_plpython

On 05.03.2019 6:45, Michael Paquier wrote:

> On Fri, Mar 01, 2019 at 05:24:39AM +0300, Nikita Glukhov wrote:
>> Unfortunately, contrib/jsonb_plpython still contain a lot of problems in error
>> handling that can lead to memory leaks:
>> - not all Python function calls are checked for the success
>> - not in all places PG exceptions are caught to release Python references
>> But it seems that this errors can happen only in OOM case.
>>
>> Attached patch with the fix. Back-patch for PG11 is needed.
> That looks right to me. Here are some comments.
>
> One thing to be really careful of when using PG_TRY/PG_CATCH blocks is
> that variables modified in the try block and then referenced in the
> catch block need to be marked as volatile. If you don't do that, the
> value when reaching the catch part is indeterminate.
>
> With your patch the result variable used in two places of
> PLyObject_FromJsonbContainer() is not marked as volatile. Similarly,
> it seems that "items" in PLyMapping_ToJsonbValue() and "seq" in
> "PLySequence_ToJsonbValue" should be volatile because they get changed
> in the try loop, and referenced afterwards.

I known about this volatility issues, but maybe I incorrectly understand what
should be marked as volatile for pointer variables: the pointer itself and/or
the memory referenced by it. I thought that only pointer needs to be marked,
and also there is message [1] clearly describing what needs to be marked.

Previously in PLyMapping_ToJsonbValue() the whole contents of PyObject was
marked as volatile, not the pointer itself which is not modified in PG_TRY:

- /* We need it volatile, since we use it after longjmp */
- volatile PyObject *items_v = NULL;

So, I removed volatile qualifier here.

Variable 'result' is also not modified in PG_TRY, it is also non-volatile.

I marked only 'key' variable in PLyObject_FromJsonbContainer() as volatile,
because it is really modified in the loop inside PG_TRY(), and
PLyObject_FromJsonbValue(&v2) call after its assignment can throw PG
exception:
+ PyObject *volatile key = NULL;

Also I have idea to introduce a global list of Python objects that need to be
dereferenced in PG_CATCH inside PLy_exec_function() in the case of exception.
Then typical code will be look like that:

PyObject *list = PLy_RegisterObject(PyList_New());

if (!list)
return NULL;

... code that can throw PG exception, PG_TRY/PG_CATCH is not needed ...

return PLy_UnregisterObject(list); /* returns list */

> Another issue: in ltree_plpython we don't check the return state of
> PyList_SetItem(), which we should complain about I think.

Yes, PyList_SetItem() and PyString_FromStringAndSize() should be checked,
but CPython's PyList_SetItem() really should not fail because list storage
is preallocated:

int
PyList_SetItem(PyObject *op, Py_ssize_t i, PyObject *newitem)
{
PyObject **p;
if (!PyList_Check(op)) {
Py_XDECREF(newitem);
PyErr_BadInternalCall();
return -1;
}
if (!valid_index(i, Py_SIZE(op))) {
Py_XDECREF(newitem);
PyErr_SetString(PyExc_IndexError,
"list assignment index out of range");
return -1;
}
p = ((PyListObject *)op) -> ob_item + i;
Py_XSETREF(*p, newitem);
return 0;
}

[1] https://www.postgresql.org/message-id/31436.1483415248%40sss.pgh.pa.us

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company