Обсуждение: Re: [COMMITTERS] pgsql: Add traceback information to PL/Python errors
On Wed, Apr 6, 2011 at 3:37 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > Add traceback information to PL/Python errors > > This mimics the traceback information the Python interpreter prints > with exceptions. > > Jan Urbański On my system this spits out a warning: plpython.c: In function ‘PLy_traceback’: plpython.c:4487: warning: ‘s’ may be used uninitialized in this function plpython.c:4487: note: ‘s’ was declared here That appears to be a live bug, unless it's guaranteed that lineno will always be > 0. Also, the loop test should really be written as current < lineno, rather than current != lineno, just in case we should manage to pass a lineno < 0, which with the current code would go into the tank and spin. This part looks pretty sketchy, too: while (s && isspace((unsigned char) *s)) s++; Perhaps we meant to test *s here. It's hard to believe that we're really intending to test whether the pointer has fallen off the end of the address space and wrapped around to NULL. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 07/04/11 23:01, Robert Haas wrote: > On Wed, Apr 6, 2011 at 3:37 PM, Peter Eisentraut <peter_e@gmx.net> wrote: >> Add traceback information to PL/Python errors >> >> This mimics the traceback information the Python interpreter prints >> with exceptions. >> >> Jan Urbański > > On my system this spits out a warning: > > plpython.c: In function ‘PLy_traceback’: > plpython.c:4487: warning: ‘s’ may be used uninitialized in this function > plpython.c:4487: note: ‘s’ was declared here > > That appears to be a live bug, unless it's guaranteed that lineno will > always be > 0. lineno should be > 0, unless Python is trying to tell us that the code frame originates from before the function. > Also, the loop test should really be written as current < lineno, > rather than current != lineno, just in case we should manage to pass a > lineno < 0, which with the current code would go into the tank and > spin. Yeah, good point. > This part looks pretty sketchy, too: > > while (s && isspace((unsigned char) *s)) > s++; > > Perhaps we meant to test *s here. It's hard to believe that we're > really intending to test whether the pointer has fallen off the end of > the address space and wrapped around to NULL. Gah, so short a function and so many things that I managed to get wrong. There's also this: http://archives.postgresql.org/pgsql-hackers/2011-04/msg00334.php Jan
On Thu, Apr 7, 2011 at 5:06 PM, Jan Urbański <wulczer@wulczer.org> wrote: > On 07/04/11 23:01, Robert Haas wrote: >> On Wed, Apr 6, 2011 at 3:37 PM, Peter Eisentraut <peter_e@gmx.net> wrote: >>> Add traceback information to PL/Python errors >>> >>> This mimics the traceback information the Python interpreter prints >>> with exceptions. >>> >>> Jan Urbański >> >> On my system this spits out a warning: >> >> plpython.c: In function ‘PLy_traceback’: >> plpython.c:4487: warning: ‘s’ may be used uninitialized in this function >> plpython.c:4487: note: ‘s’ was declared here >> >> That appears to be a live bug, unless it's guaranteed that lineno will >> always be > 0. > > lineno should be > 0, unless Python is trying to tell us that the code > frame originates from before the function. > >> Also, the loop test should really be written as current < lineno, >> rather than current != lineno, just in case we should manage to pass a >> lineno < 0, which with the current code would go into the tank and >> spin. > > Yeah, good point. > >> This part looks pretty sketchy, too: >> >> while (s && isspace((unsigned char) *s)) >> s++; >> >> Perhaps we meant to test *s here. It's hard to believe that we're >> really intending to test whether the pointer has fallen off the end of >> the address space and wrapped around to NULL. > > Gah, so short a function and so many things that I managed to get wrong. Patch? > There's also this: > http://archives.postgresql.org/pgsql-hackers/2011-04/msg00334.php Yep. I am assuming Peter will look at that one. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 08/04/11 00:25, Robert Haas wrote: > On Thu, Apr 7, 2011 at 5:06 PM, Jan Urbański <wulczer@wulczer.org> wrote: >> On 07/04/11 23:01, Robert Haas wrote: >>> On Wed, Apr 6, 2011 at 3:37 PM, Peter Eisentraut <peter_e@gmx.net> wrote: >>>> Add traceback information to PL/Python errors >>>> >>>> This mimics the traceback information the Python interpreter prints >>>> with exceptions. >>>> >>>> Jan Urbański >>> >>> On my system this spits out a warning: >>> >>> plpython.c: In function ‘PLy_traceback’: >>> plpython.c:4487: warning: ‘s’ may be used uninitialized in this function >>> plpython.c:4487: note: ‘s’ was declared here >>> >>> That appears to be a live bug, unless it's guaranteed that lineno will >>> always be > 0. >> >> lineno should be > 0, unless Python is trying to tell us that the code >> frame originates from before the function. >> >>> Also, the loop test should really be written as current < lineno, >>> rather than current != lineno, just in case we should manage to pass a >>> lineno < 0, which with the current code would go into the tank and >>> spin. >> >> Yeah, good point. >> >>> This part looks pretty sketchy, too: >>> >>> while (s && isspace((unsigned char) *s)) >>> s++; >>> >>> Perhaps we meant to test *s here. It's hard to believe that we're >>> really intending to test whether the pointer has fallen off the end of >>> the address space and wrapped around to NULL. >> >> Gah, so short a function and so many things that I managed to get wrong. > > Patch? Attached. >> There's also this: >> http://archives.postgresql.org/pgsql-hackers/2011-04/msg00334.php > > Yep. I am assuming Peter will look at that one. I guess so. This only fixes the things you noticed. Jan