Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On Thu, Feb 23, 2012 at 8:35 PM, Daniele Varrazzo daniele.varra...@gmail.com wrote: On Mon, Feb 20, 2012 at 12:09 AM, Jan Urbański wulc...@wulczer.org wrote: BTW, that tool is quite handy, I'll have to try running it over psycopg2. Indeed. I'm having a play with it. It is reporting several issues to clean up (mostly on failure at module import). It's also tracebacking here and there: I'll send the author some feedback/patches. I'm patching psycopg in the gcc-python-plugin branch in my dev repos (https://github.com/dvarrazzo/psycopg). The just released psycopg 2.4.5 has had its codebase cleaned up using the gcc static checker. I haven't managed to run it without false positives, however it's been very useful to find missing return value checks and other nuances. No live memory leak has been found: there were a few missing decrefs but only at module init, not in the live code. Full report about the changes in this message: https://fedorahosted.org/pipermail/gcc-python-plugin/2012-March/000229.html. Cheers, -- Daniele -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= wulc...@wulczer.org writes: Here are the updated patches which use PLy_elog instead of plain elog. The difference is that they will get marked for translation and that the original Python exception will show up in the errdetail field. Applied, thanks. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On Mon, Feb 20, 2012 at 12:09 AM, Jan Urbański wulc...@wulczer.org wrote: BTW, that tool is quite handy, I'll have to try running it over psycopg2. Indeed. I'm having a play with it. It is reporting several issues to clean up (mostly on failure at module import). It's also tracebacking here and there: I'll send the author some feedback/patches. I'm patching psycopg in the gcc-python-plugin branch in my dev repos (https://github.com/dvarrazzo/psycopg). -- Daniele -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On 21/02/12 18:28, Jan Urbański wrote:
On 21/02/12 18:05, Peter Eisentraut wrote:
it might be better to use ereport, to expose the message
for translation.
After giving it some thought some of these elogs could be changed into
PLy_elogs (which is meant to propagate a Python error into Postgres) and
the others made into ereports.
I'll send updated patches this evening (CET).
Inevitably, this turned into the next morning CET.
Here are the updated patches which use PLy_elog instead of plain elog.
The difference is that they will get marked for translation and that the
original Python exception will show up in the errdetail field.
Cheers,
Jan
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
index 741980c..332898d 100644
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*** get_source_line(const char *src, int lin
*** 365,370
--- 365,374
const char *next = src;
int current = 0;
+ /* sanity check */
+ if (lineno = 0)
+ return NULL;
+
while (current lineno)
{
s = next;
diff --git a/src/pl/plpython/plpy_main.c b/src/pl/plpython/plpy_main.c
index ae9d87e..f259ecf 100644
*** a/src/pl/plpython/plpy_main.c
--- b/src/pl/plpython/plpy_main.c
*** PLy_init_interp(void)
*** 133,138
--- 133,140
Py_INCREF(mainmod);
PLy_interp_globals = PyModule_GetDict(mainmod);
PLy_interp_safe_globals = PyDict_New();
+ if (PLy_interp_safe_globals == NULL)
+ PLy_elog(ERROR, could not create globals);
PyDict_SetItemString(PLy_interp_globals, GD, PLy_interp_safe_globals);
Py_DECREF(mainmod);
if (PLy_interp_globals == NULL || PyErr_Occurred())
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
index d2d0a2a..0c96f03 100644
*** a/src/pl/plpython/plpy_plpymodule.c
--- b/src/pl/plpython/plpy_plpymodule.c
*** PLy_init_plpy(void)
*** 173,181
main_mod = PyImport_AddModule(__main__);
main_dict = PyModule_GetDict(main_mod);
plpy_mod = PyImport_AddModule(plpy);
PyDict_SetItemString(main_dict, plpy, plpy_mod);
if (PyErr_Occurred())
! elog(ERROR, could not initialize plpy);
}
static void
--- 173,183
main_mod = PyImport_AddModule(__main__);
main_dict = PyModule_GetDict(main_mod);
plpy_mod = PyImport_AddModule(plpy);
+ if (plpy_mod == NULL)
+ PLy_elog(ERROR, could not initialize plpy);
PyDict_SetItemString(main_dict, plpy, plpy_mod);
if (PyErr_Occurred())
! PLy_elog(ERROR, could not initialize plpy);
}
static void
*** PLy_add_exceptions(PyObject *plpy)
*** 208,213
--- 210,220
PLy_exc_fatal = PyErr_NewException(plpy.Fatal, NULL, NULL);
PLy_exc_spi_error = PyErr_NewException(plpy.SPIError, NULL, NULL);
+ if (PLy_exc_error == NULL ||
+ PLy_exc_fatal == NULL ||
+ PLy_exc_spi_error == NULL)
+ PLy_elog(ERROR, could not create the base SPI exceptions);
+
Py_INCREF(PLy_exc_error);
PyModule_AddObject(plpy, Error, PLy_exc_error);
Py_INCREF(PLy_exc_fatal);
*** PLy_generate_spi_exceptions(PyObject *mo
*** 241,247
--- 248,260
PyObject *sqlstate;
PyObject *dict = PyDict_New();
+ if (dict == NULL)
+ PLy_elog(ERROR, could not generate SPI exceptions);
+
sqlstate = PyString_FromString(unpack_sql_state(exception_map[i].sqlstate));
+ if (sqlstate == NULL)
+ PLy_elog(ERROR, could not generate SPI exceptions);
+
PyDict_SetItemString(dict, sqlstate, sqlstate);
Py_DECREF(sqlstate);
exc = PyErr_NewException(exception_map[i].name, base, dict);
*** PLy_output(volatile int level, PyObject
*** 369,376
* decoration.
*/
PyObject *o;
- PyArg_UnpackTuple(args, plpy.elog, 1, 1, o);
so = PyObject_Str(o);
}
else
--- 382,393
* decoration.
*/
PyObject *o;
+ int result;
+
+ result = PyArg_UnpackTuple(args, plpy.elog, 1, 1, o);
+ if (!result)
+ PLy_elog(ERROR, could not unpack arguments in plpy.elog);
so = PyObject_Str(o);
}
else
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 0d63c4f..20b93cd 100644
*** a/src/pl/plpython/plpy_spi.c
--- b/src/pl/plpython/plpy_spi.c
*** PLy_spi_execute_query(char *query, long
*** 338,344
int rv;
volatile MemoryContext oldcontext;
volatile ResourceOwner oldowner;
! PyObject *ret;
oldcontext = CurrentMemoryContext;
oldowner = CurrentResourceOwner;
--- 338,344
int rv;
volatile MemoryContext oldcontext;
volatile ResourceOwner oldowner;
! PyObject *ret = NULL;
oldcontext = CurrentMemoryContext;
oldowner = CurrentResourceOwner;
*** PLy_spi_execute_query(char *query, long
*** 362,367
--- 362,368
if (rv 0)
{
+ Py_XDECREF(ret);
PLy_exception_set(PLy_exc_spi_error,
SPI_execute failed: %s,
SPI_result_code_string(rv));
diff --git a/src/pl/plpython/plpy_typeio.c
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On sön, 2012-02-19 at 22:29 -0500, Tom Lane wrote: My only comment is whether elog(ERROR) is appropriate, ie, do we consider these to be internal errors that users will never see in practice? If there's a significant risk of the error being thrown in the field, it might be better to use ereport, to expose the message for translation. I find the wording of the error messages a bit inappropriate. For example, list = PyList_New(length); + if (list == NULL) + elog(ERROR, could not transform Python list to array); The error is not about the transforming, it's about creating a new list. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On 21/02/12 18:05, Peter Eisentraut wrote: On sön, 2012-02-19 at 22:29 -0500, Tom Lane wrote: My only comment is whether elog(ERROR) is appropriate, ie, do we consider these to be internal errors that users will never see in practice? If there's a significant risk of the error being thrown in the field, it might be better to use ereport, to expose the message for translation. I find the wording of the error messages a bit inappropriate. For example, list = PyList_New(length); + if (list == NULL) + elog(ERROR, could not transform Python list to array); The error is not about the transforming, it's about creating a new list. Well, what I tried to convery here was that the process of transforming a Python list to a Postgres array failed. Which, by the way, is wrong since this function transforms a Postgres array to a Python list... After giving it some thought some of these elogs could be changed into PLy_elogs (which is meant to propagate a Python error into Postgres) and the others made into ereports. I'll send updated patches this evening (CET). Cheers, Jan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On 20/02/12 04:29, Tom Lane wrote: =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= wulc...@wulczer.org writes: On 18/02/12 21:17, Tom Lane wrote: Dave Malcolm at Red Hat has been working on a static code analysis tool for Python-related C code. He reports here on some preliminary results for plpython.c: https://bugzilla.redhat.com/show_bug.cgi?id=795011 Here's a patch that fixes everything I was sure was an actual bug. The rest of the warnings seem to be caused by the tool not knowing that elog(ERROR) throws a longjmp and things like we never unref this object, so it can't disappear mid-execution. My only comment is whether elog(ERROR) is appropriate, ie, do we consider these to be internal errors that users will never see in practice? AFAICS these errors can only happen on out of memory conditions or other internal errors (like trying to create a list with a negative length). Cheers, Jan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On Mon, Feb 20, 2012 at 5:05 AM, Jan Urbański wulc...@wulczer.org wrote: On 20/02/12 04:29, Tom Lane wrote: =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= wulc...@wulczer.org writes: On 18/02/12 21:17, Tom Lane wrote: Dave Malcolm at Red Hat has been working on a static code analysis tool for Python-related C code. He reports here on some preliminary results for plpython.c: https://bugzilla.redhat.com/show_bug.cgi?id=795011 Here's a patch that fixes everything I was sure was an actual bug. The rest of the warnings seem to be caused by the tool not knowing that elog(ERROR) throws a longjmp and things like we never unref this object, so it can't disappear mid-execution. My only comment is whether elog(ERROR) is appropriate, ie, do we consider these to be internal errors that users will never see in practice? AFAICS these errors can only happen on out of memory conditions or other internal errors (like trying to create a list with a negative length). We typically want out of memory to use ereport, so that it gets translated. For example, in fd.c we have: ereport(FATAL, (errcode(ERRCODE_OUT_OF_MEMORY), errmsg(out of memory))); Trying to create a list with a negative length sounds similar. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On 18/02/12 21:18, Jan Urbański wrote:
On 18/02/12 21:17, Tom Lane wrote:
=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= wulc...@wulczer.org writes:
On 18/02/12 20:30, Tom Lane wrote:
Dave Malcolm at Red Hat has been working on a static code analysis tool
for Python-related C code. He reports here on some preliminary results
for plpython.c:
https://bugzilla.redhat.com/show_bug.cgi?id=795011
If you find any live bugs, it'd likely be better to deal with them as
a separate patch so that we can back-patch ...
Sure, I meant to say I'll look at these as well, but will make them into
a separate patch.
Here's a patch that fixes everything I was sure was an actual bug. The
rest of the warnings seem to be caused by the tool not knowing that
elog(ERROR) throws a longjmp and things like we never unref this
object, so it can't disappear mid-execution.
Attached are patches for HEAD and for 9.1.x (since splitting plpython.c
in 9.2 was kind of my idea I felt bad about you having to back-patch
this so I tried to do the necessary legwork myself; I hope the attached
is what you need).
BTW, that tool is quite handy, I'll have to try running it over psycopg2.
Cheers,
Jan
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 530b5f0..2b064c5 100644
*** a/src/pl/plpython/plpython.c
--- b/src/pl/plpython/plpython.c
*** PLyList_FromArray(PLyDatumToOb *arg, Dat
*** 2345,2350
--- 2345,2352
length = ARR_DIMS(array)[0];
lbound = ARR_LBOUND(array)[0];
list = PyList_New(length);
+ if (list == NULL)
+ elog(ERROR, could not transform Python list to array);
for (i = 0; i length; i++)
{
*** PLy_spi_execute_query(char *query, long
*** 3664,3670
int rv;
volatile MemoryContext oldcontext;
volatile ResourceOwner oldowner;
! PyObject *ret;
oldcontext = CurrentMemoryContext;
oldowner = CurrentResourceOwner;
--- 3666,3672
int rv;
volatile MemoryContext oldcontext;
volatile ResourceOwner oldowner;
! PyObject *ret = NULL;
oldcontext = CurrentMemoryContext;
oldowner = CurrentResourceOwner;
*** PLy_spi_execute_query(char *query, long
*** 3727,3732
--- 3729,3735
if (rv 0)
{
+ Py_XDECREF(ret);
PLy_exception_set(PLy_exc_spi_error,
SPI_execute failed: %s,
SPI_result_code_string(rv));
*** PLy_generate_spi_exceptions(PyObject *mo
*** 3967,3973
--- 3970,3982
PyObject *sqlstate;
PyObject *dict = PyDict_New();
+ if (dict == NULL)
+ elog(ERROR, could not generate SPI exceptions);
+
sqlstate = PyString_FromString(unpack_sql_state(exception_map[i].sqlstate));
+ if (sqlstate == NULL)
+ elog(ERROR, could not generate SPI exceptions);
+
PyDict_SetItemString(dict, sqlstate, sqlstate);
Py_DECREF(sqlstate);
exc = PyErr_NewException(exception_map[i].name, base, dict);
*** PLy_add_exceptions(PyObject *plpy)
*** 4008,4013
--- 4017,4027
PLy_exc_fatal = PyErr_NewException(plpy.Fatal, NULL, NULL);
PLy_exc_spi_error = PyErr_NewException(plpy.SPIError, NULL, NULL);
+ if (PLy_exc_error == NULL ||
+ PLy_exc_fatal == NULL ||
+ PLy_exc_spi_error == NULL)
+ elog(ERROR, could not create the base SPI exceptions);
+
Py_INCREF(PLy_exc_error);
PyModule_AddObject(plpy, Error, PLy_exc_error);
Py_INCREF(PLy_exc_fatal);
*** PLy_init_interp(void)
*** 4124,4129
--- 4138,4145
Py_INCREF(mainmod);
PLy_interp_globals = PyModule_GetDict(mainmod);
PLy_interp_safe_globals = PyDict_New();
+ if (PLy_interp_safe_globals == NULL)
+ PLy_elog(ERROR, could not create globals);
PyDict_SetItemString(PLy_interp_globals, GD, PLy_interp_safe_globals);
Py_DECREF(mainmod);
if (PLy_interp_globals == NULL || PyErr_Occurred())
*** PLy_init_plpy(void)
*** 4164,4169
--- 4180,4187
main_mod = PyImport_AddModule(__main__);
main_dict = PyModule_GetDict(main_mod);
plpy_mod = PyImport_AddModule(plpy);
+ if (plpy_mod == NULL)
+ elog(ERROR, could not initialize plpy);
PyDict_SetItemString(main_dict, plpy, plpy_mod);
if (PyErr_Occurred())
elog(ERROR, could not initialize plpy);
*** PLy_output(volatile int level, PyObject
*** 4231,4238
* decoration.
*/
PyObject *o;
- PyArg_UnpackTuple(args, plpy.elog, 1, 1, o);
so = PyObject_Str(o);
}
else
--- 4249,4260
* decoration.
*/
PyObject *o;
+ int result;
+
+ result = PyArg_UnpackTuple(args, plpy.elog, 1, 1, o);
+ if (!result)
+ elog(ERROR, could not unpack arguments in plpy.elog);
so = PyObject_Str(o);
}
else
*** get_source_line(const char *src, int lin
*** 4554,4559
--- 4576,4585
const char *next = src;
int current = 0;
+ /* sanity check */
+ if (lineno = 0)
+ return NULL;
+
while (current lineno)
{
s = next;
diff --git a/src/pl/plpython/plpy_elog.c
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= wulc...@wulczer.org writes: On 18/02/12 21:17, Tom Lane wrote: Dave Malcolm at Red Hat has been working on a static code analysis tool for Python-related C code. He reports here on some preliminary results for plpython.c: https://bugzilla.redhat.com/show_bug.cgi?id=795011 Here's a patch that fixes everything I was sure was an actual bug. The rest of the warnings seem to be caused by the tool not knowing that elog(ERROR) throws a longjmp and things like we never unref this object, so it can't disappear mid-execution. This looks pretty sane to me, but it would probably be better if one of the more python-savvy committers took responsibility for final review. My only comment is whether elog(ERROR) is appropriate, ie, do we consider these to be internal errors that users will never see in practice? If there's a significant risk of the error being thrown in the field, it might be better to use ereport, to expose the message for translation. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Potential reference miscounts and segfaults in plpython.c
Dave Malcolm at Red Hat has been working on a static code analysis tool for Python-related C code. He reports here on some preliminary results for plpython.c: https://bugzilla.redhat.com/show_bug.cgi?id=795011 I'm not enough of a Python hacker to evaluate the significance of these issues, but somebody who does work on that code ought to take a look and see what we ought to patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On 18/02/12 20:30, Tom Lane wrote: Dave Malcolm at Red Hat has been working on a static code analysis tool for Python-related C code. He reports here on some preliminary results for plpython.c: https://bugzilla.redhat.com/show_bug.cgi?id=795011 I'm not enough of a Python hacker to evaluate the significance of these issues, but somebody who does work on that code ought to take a look and see what we ought to patch. Very cool! Some of them look like legitimate bugs, some seem to stem from the fact that the tool does not know that PLy_elog(ERROR) does not return. I wonder if it could be taught that. I'll try to fixes these while reworking the I/O functions memory leak patch I sent earlier. Cheers, Jan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= wulc...@wulczer.org writes: On 18/02/12 20:30, Tom Lane wrote: Dave Malcolm at Red Hat has been working on a static code analysis tool for Python-related C code. He reports here on some preliminary results for plpython.c: https://bugzilla.redhat.com/show_bug.cgi?id=795011 I'll try to fixes these while reworking the I/O functions memory leak patch I sent earlier. If you find any live bugs, it'd likely be better to deal with them as a separate patch so that we can back-patch ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Potential reference miscounts and segfaults in plpython.c
On 18/02/12 21:17, Tom Lane wrote: =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= wulc...@wulczer.org writes: On 18/02/12 20:30, Tom Lane wrote: Dave Malcolm at Red Hat has been working on a static code analysis tool for Python-related C code. He reports here on some preliminary results for plpython.c: https://bugzilla.redhat.com/show_bug.cgi?id=795011 I'll try to fixes these while reworking the I/O functions memory leak patch I sent earlier. If you find any live bugs, it'd likely be better to deal with them as a separate patch so that we can back-patch ... Sure, I meant to say I'll look at these as well, but will make them into a separate patch. Cheers, Jan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
