Fix memleaks and error handling in jsonb_plpython
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.
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
0001-Fix-memleaks-and-error-handling-in-jsonb_plpython-v01.patchtext/x-patch; name=0001-Fix-memleaks-and-error-handling-in-jsonb_plpython-v01.patchDownload
From e40a9012c38c3b66888791d3dd3943adf9f310c8 Mon Sep 17 00:00:00 2001
From: Nikita Glukhov <n.gluhov@postgrespro.ru>
Date: Tue, 15 Jan 2019 02:14:06 +0300
Subject: [PATCH] Fix memleaks and error handling in jsonb_plpython
---
contrib/jsonb_plpython/jsonb_plpython.c | 146 ++++++++++++++++++++++----------
1 file changed, 100 insertions(+), 46 deletions(-)
diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c
index f44d364..3143b73 100644
--- a/contrib/jsonb_plpython/jsonb_plpython.c
+++ b/contrib/jsonb_plpython/jsonb_plpython.c
@@ -169,53 +169,80 @@ PLyObject_FromJsonbContainer(JsonbContainer *jsonb)
if (!result)
return NULL;
- while ((r = JsonbIteratorNext(&it, &v, true)) != WJB_DONE)
+ PG_TRY();
{
- if (r == WJB_ELEM)
+ while ((r = JsonbIteratorNext(&it, &v, true)) != WJB_DONE)
{
- PyObject *elem = PLyObject_FromJsonbValue(&v);
+ if (r == WJB_ELEM)
+ {
+ PyObject *elem = PLyObject_FromJsonbValue(&v);
+
+ if (!elem || PyList_Append(result, elem))
+ {
+ Py_XDECREF(elem);
+ Py_DECREF(result);
+ return NULL;
+ }
- PyList_Append(result, elem);
- Py_XDECREF(elem);
+ Py_DECREF(elem);
+ }
}
}
+ PG_CATCH();
+ {
+ Py_DECREF(result);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
}
break;
case WJB_BEGIN_OBJECT:
- result = PyDict_New();
- if (!result)
- return NULL;
-
- while ((r = JsonbIteratorNext(&it, &v, true)) != WJB_DONE)
{
- if (r == WJB_KEY)
- {
- PyObject *key = PLyString_FromJsonbValue(&v);
-
- if (!key)
- return NULL;
+ PyObject *volatile key = NULL;
- r = JsonbIteratorNext(&it, &v, true);
+ result = PyDict_New();
+ if (!result)
+ return NULL;
- if (r == WJB_VALUE)
+ PG_TRY();
+ {
+ while ((r = JsonbIteratorNext(&it, &v, true)) != WJB_DONE)
{
- PyObject *value = PLyObject_FromJsonbValue(&v);
+ JsonbValue v2;
+ PyObject *val = NULL;
+
+ if (r != WJB_KEY)
+ continue;
+
+ if ((r = JsonbIteratorNext(&it, &v2, true)) != WJB_VALUE)
+ elog(ERROR, "unexpected jsonb token: %d", r);
- if (!value)
+ if (!(key = PLyString_FromJsonbValue(&v)) ||
+ !(val = PLyObject_FromJsonbValue(&v2)) ||
+ PyDict_SetItem(result, key, val))
{
Py_XDECREF(key);
+ Py_XDECREF(val);
+ Py_DECREF(result);
return NULL;
}
- PyDict_SetItem(result, key, value);
- Py_XDECREF(value);
+ Py_DECREF(val);
+ Py_DECREF(key);
+ key = NULL;
}
-
+ }
+ PG_CATCH();
+ {
Py_XDECREF(key);
+ Py_DECREF(result);
+ PG_RE_THROW();
}
+ PG_END_TRY();
+
+ break;
}
- break;
default:
elog(ERROR, "unexpected jsonb token: %d", r);
@@ -233,30 +260,40 @@ PLyObject_FromJsonbContainer(JsonbContainer *jsonb)
static JsonbValue *
PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
{
- Py_ssize_t pcount;
- JsonbValue *out = NULL;
+ JsonbValue *out;
+ PyObject *items;
+ Py_ssize_t pcount = PyMapping_Size(obj);
+
+ if (pcount < 0)
+ PLy_elog(ERROR, "PyMapping_Size() failed, while converting mapping into jsonb");
- /* We need it volatile, since we use it after longjmp */
- volatile PyObject *items_v = NULL;
+ items = PyMapping_Items(obj);
- pcount = PyMapping_Size(obj);
- items_v = PyMapping_Items(obj);
+ if (!items)
+ PLy_elog(ERROR, "PyMapping_Items() failed, while converting mapping into jsonb");
PG_TRY();
{
Py_ssize_t i;
- PyObject *items;
-
- items = (PyObject *) items_v;
pushJsonbValue(jsonb_state, WJB_BEGIN_OBJECT, NULL);
for (i = 0; i < pcount; i++)
{
JsonbValue jbvKey;
- PyObject *item = PyList_GetItem(items, i);
- PyObject *key = PyTuple_GetItem(item, 0);
- PyObject *value = PyTuple_GetItem(item, 1);
+ PyObject *item;
+ PyObject *key;
+ PyObject *value;
+
+ item = PyList_GetItem(items, i); /* borrowed reference */
+ if (!item)
+ PLy_elog(ERROR, "PyList_GetItem() failed, while converting mapping into jsonb");
+
+ key = PyTuple_GetItem(item, 0); /* borrowed references */
+ value = PyTuple_GetItem(item, 1);
+
+ if (!key || !value)
+ PLy_elog(ERROR, "PyTuple_GetItem() failed, while converting mapping into jsonb");
/* Python dictionary can have None as key */
if (key == Py_None)
@@ -279,11 +316,13 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(items);
PG_RE_THROW();
}
PG_END_TRY();
+ Py_DECREF(items);
+
return out;
}
@@ -296,21 +335,36 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
static JsonbValue *
PLySequence_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
{
- Py_ssize_t i;
- Py_ssize_t pcount;
+ PyObject *seq = PySequence_Fast(obj, "object is not a sequence");
- pcount = PySequence_Size(obj);
+ if (!seq)
+ PLy_elog(ERROR, "PySequence_Fast() failed, while converting sequence into jsonb");
- pushJsonbValue(jsonb_state, WJB_BEGIN_ARRAY, NULL);
-
- for (i = 0; i < pcount; i++)
+ PG_TRY();
{
- PyObject *value = PySequence_GetItem(obj, i);
+ Py_ssize_t i;
+ Py_ssize_t pcount = PySequence_Fast_GET_SIZE(seq);
- (void) PLyObject_ToJsonbValue(value, jsonb_state, true);
+ pushJsonbValue(jsonb_state, WJB_BEGIN_ARRAY, NULL);
- Py_XDECREF(value);
+ for (i = 0; i < pcount; i++)
+ {
+ PyObject *value = PySequence_Fast_GET_ITEM(seq, i); /* borrowed reference */
+
+ if (!value)
+ PLy_elog(ERROR, "PySequence_Fast_GET_ITEM() failed, while converting sequence into jsonb");
+
+ (void) PLyObject_ToJsonbValue(value, jsonb_state, true);
+ }
}
+ PG_CATCH();
+ {
+ Py_DECREF(seq);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ Py_DECREF(seq);
return pushJsonbValue(jsonb_state, WJB_END_ARRAY, NULL);
}
--
2.7.4
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.
Another issue: in ltree_plpython we don't check the return state of
PyList_SetItem(), which we should complain about I think.
--
Michael
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]/messages/by-id/31436.1483415248@sss.pgh.pa.us 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]: /messages/by-id/31436.1483415248@sss.pgh.pa.us
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
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
On Wed, Mar 06, 2019 at 11:04:23AM +0900, Michael Paquier wrote:
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.
So, I have been poking at this stuff, and I am finishing with the
attached. The origin of the issue comes from PLyObject_ToJsonbValue()
and PLyObject_FromJsonbValue() which could result in problems when
working on PyObject which it may allocate. So this has resulted in
more refactoring of the code than I expected first. I also decided to
not keep the additional errors which have been added in the previous
version of the patch. From my understanding of the code, these cannot
actually happen, so replacing them by assertions is enough in my
opinion.
While on it, I also noticed that hstore_plpython does not actually
need a volatile pointer for plpython_to_hstore(). Also, as all those
problems are really unlikely going to happen in real-life cases,
improving this code only on HEAD looks enough to me.
--
Michael
Attachments:
plpython-leaks.patchtext/x-diff; charset=us-asciiDownload
diff --git a/contrib/hstore_plpython/hstore_plpython.c b/contrib/hstore_plpython/hstore_plpython.c
index 2f24090ff3..f1469043bd 100644
--- a/contrib/hstore_plpython/hstore_plpython.c
+++ b/contrib/hstore_plpython/hstore_plpython.c
@@ -128,7 +128,7 @@ Datum
plpython_to_hstore(PG_FUNCTION_ARGS)
{
PyObject *dict;
- volatile PyObject *items_v = NULL;
+ PyObject *items = NULL;
int32 pcount;
HStore *out;
@@ -139,14 +139,13 @@ plpython_to_hstore(PG_FUNCTION_ARGS)
errmsg("not a Python mapping")));
pcount = PyMapping_Size(dict);
- items_v = PyMapping_Items(dict);
+ items = PyMapping_Items(dict);
PG_TRY();
{
int32 buflen;
int32 i;
Pairs *pairs;
- PyObject *items = (PyObject *) items_v;
pairs = palloc(pcount * sizeof(*pairs));
@@ -177,17 +176,18 @@ plpython_to_hstore(PG_FUNCTION_ARGS)
pairs[i].isnull = false;
}
}
- Py_DECREF(items_v);
pcount = hstoreUniquePairs(pairs, pcount, &buflen);
out = hstorePairs(pairs, pcount, buflen);
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(items);
PG_RE_THROW();
}
PG_END_TRY();
+ Py_DECREF(items);
+
PG_RETURN_POINTER(out);
}
diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c
index f44d364c97..054e9594d0 100644
--- a/contrib/jsonb_plpython/jsonb_plpython.c
+++ b/contrib/jsonb_plpython/jsonb_plpython.c
@@ -164,56 +164,92 @@ PLyObject_FromJsonbContainer(JsonbContainer *jsonb)
}
else
{
- /* array in v */
+ /* volatile as it gets used after longjmp */
+ PyObject *volatile elem = NULL;
+
result = PyList_New(0);
if (!result)
return NULL;
- while ((r = JsonbIteratorNext(&it, &v, true)) != WJB_DONE)
+ PG_TRY();
{
- if (r == WJB_ELEM)
+ while ((r = JsonbIteratorNext(&it, &v, true)) != WJB_DONE)
{
- PyObject *elem = PLyObject_FromJsonbValue(&v);
+ if (r != WJB_ELEM)
+ continue;
+
+ elem = PLyObject_FromJsonbValue(&v);
PyList_Append(result, elem);
Py_XDECREF(elem);
+ elem = NULL;
}
}
+ PG_CATCH();
+ {
+ Py_XDECREF(elem);
+ Py_XDECREF(result);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
}
break;
case WJB_BEGIN_OBJECT:
- result = PyDict_New();
- if (!result)
- return NULL;
-
- while ((r = JsonbIteratorNext(&it, &v, true)) != WJB_DONE)
{
- if (r == WJB_KEY)
+ /* volatile as it gets used after longjmp */
+ PyObject *volatile result_v = PyDict_New();
+ PyObject *volatile key = NULL;
+ PyObject *volatile val = NULL;
+
+ if (!result_v)
+ return NULL;
+
+ PG_TRY();
{
- PyObject *key = PLyString_FromJsonbValue(&v);
-
- if (!key)
- return NULL;
-
- r = JsonbIteratorNext(&it, &v, true);
-
- if (r == WJB_VALUE)
+ while ((r = JsonbIteratorNext(&it, &v, true)) != WJB_DONE)
{
- PyObject *value = PLyObject_FromJsonbValue(&v);
+ if (r != WJB_KEY)
+ continue;
- if (!value)
+ key = PLyString_FromJsonbValue(&v);
+ if (!key)
{
- Py_XDECREF(key);
- return NULL;
+ Py_XDECREF(result_v);
+ result_v = NULL;
+ break;
}
- PyDict_SetItem(result, key, value);
- Py_XDECREF(value);
- }
+ if (JsonbIteratorNext(&it, &v, true) != WJB_VALUE)
+ elog(ERROR, "unexpected jsonb token: %d", r);
- Py_XDECREF(key);
+ val = PLyObject_FromJsonbValue(&v);
+ if (!val)
+ {
+ Py_XDECREF(key);
+ Py_XDECREF(result_v);
+ result_v = NULL;
+ break;
+ }
+
+ PyDict_SetItem(result_v, key, val);
+
+ Py_XDECREF(val);
+ Py_XDECREF(key);
+ key = NULL;
+ val = NULL;
+ }
}
+ PG_CATCH();
+ {
+ Py_XDECREF(result_v);
+ Py_XDECREF(key);
+ Py_XDECREF(val);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ result = result_v;
}
break;
@@ -235,19 +271,15 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
{
Py_ssize_t pcount;
JsonbValue *out = NULL;
-
- /* We need it volatile, since we use it after longjmp */
- volatile PyObject *items_v = NULL;
+ PyObject *items;
pcount = PyMapping_Size(obj);
- items_v = PyMapping_Items(obj);
+ items = PyMapping_Items(obj);
+ Assert (pcount >= 0 && items);
PG_TRY();
{
Py_ssize_t i;
- PyObject *items;
-
- items = (PyObject *) items_v;
pushJsonbValue(jsonb_state, WJB_BEGIN_OBJECT, NULL);
@@ -279,11 +311,13 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(items);
PG_RE_THROW();
}
PG_END_TRY();
+ Py_DECREF(items);
+
return out;
}
@@ -298,19 +332,31 @@ PLySequence_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
{
Py_ssize_t i;
Py_ssize_t pcount;
+ /* volatile as it gets used after longjmp */
+ PyObject *volatile value = NULL;
pcount = PySequence_Size(obj);
pushJsonbValue(jsonb_state, WJB_BEGIN_ARRAY, NULL);
- for (i = 0; i < pcount; i++)
+ PG_TRY();
{
- PyObject *value = PySequence_GetItem(obj, i);
+ for (i = 0; i < pcount; i++)
+ {
+ value = PySequence_GetItem(obj, i);
+ Assert(value);
- (void) PLyObject_ToJsonbValue(value, jsonb_state, true);
-
- Py_XDECREF(value);
+ (void) PLyObject_ToJsonbValue(value, jsonb_state, true);
+ Py_XDECREF(value);
+ value = NULL;
+ }
}
+ PG_CATCH();
+ {
+ Py_XDECREF(value);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
return pushJsonbValue(jsonb_state, WJB_END_ARRAY, NULL);
}
Michael Paquier <michael@paquier.xyz> writes:
On Wed, Mar 06, 2019 at 11:04:23AM +0900, Michael Paquier wrote:
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.
So, I have been poking at this stuff, and I am finishing with the
attached.
This patch had bit-rotted due to somebody else fooling with the
volatile-qualifiers situation. I fixed it up, tweaked a couple of
things, and pushed it.
Also, as all those
problems are really unlikely going to happen in real-life cases,
improving this code only on HEAD looks enough to me.
Yeah, I concur.
regards, tom lane