Обсуждение: 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
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
Jan Urbański <wulczer@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
On 18/02/12 21:17, Tom Lane wrote: > Jan Urbański <wulczer@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
On 18/02/12 21:18, Jan Urbański wrote: > On 18/02/12 21:17, Tom Lane wrote: >> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@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
Вложения
Jan Urbański <wulczer@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
On 20/02/12 04:29, Tom Lane wrote: > Jan Urbański <wulczer@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
On Mon, Feb 20, 2012 at 5:05 AM, Jan Urbański <wulczer@wulczer.org> wrote: > On 20/02/12 04:29, Tom Lane wrote: >> Jan Urbański <wulczer@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
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.
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
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
Вложения
On Mon, Feb 20, 2012 at 12:09 AM, Jan Urbański <wulczer@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
Jan Urbański <wulczer@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
On Thu, Feb 23, 2012 at 8:35 PM, Daniele Varrazzo <daniele.varrazzo@gmail.com> wrote: > On Mon, Feb 20, 2012 at 12:09 AM, Jan Urbański <wulczer@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