Re: Fix memleaks and error handling in jsonb_plpython

On Tue, Mar 05, 2019 at 02:10:01PM +0300, Nikita Glukhov wrote:
> 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.

Yeah, sorry for bringing some confusion.

> 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.

Okay, this one looks correct to me. Well the whole variable has been
removed.

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

Fine here as well.

> 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;

One thing that you are missing here is that key can become NULL when
reaching the catch block, so Py_XDECREF() should be called on it only
when the value is not NULL. And actually, looking closer, you don't
need to have that volatile variable at all, no? Why not just
declaring it as a PyObject in the while loop?

Also here, key and val can be NULL, so we had better only call
Py_XDECREF() when they are not. On top of that, potential errors on
PyDict_SetItem() not be simply ignored, so the loop should only break
when the key or the value is NULL, but not when PyDict_SetItem() has a
problem.

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

Perhaps we could do that, but let's not juggle with the code more than
necessary for a bug fix.

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

Hm. We could add an elog() here for safety I think. That's not a big
deal either.

Another thing is that you cannot just return within a try block with
what is added in PLyObject_FromJsonbContainer, or the error stack is
not reset properly. So they should be replaced by breaks.
--
Michael