Thread: plpython crash on exception
Following function crashes plpython on x86-64 / gcc 4.1.2 / debian 4.0:
CREATE FUNCTION crashme(str_len integer)
RETURNS text AS $$
raise Exception("X" * str_len)
$$ LANGUAGE plpythonu;
SELECT crashme(1000);
Problem turns out to be va_list handling in PLy_vprintf() which
uses same va_list repeatedly. Fix is to va_copy to temp variable.
Additionally the atteched patch fixes 2 more problems in that function:
- its nonsensical to check existing buffer length for >0, instead the
function result should be checked. (which for vsnprintf() should
always be > 0, but maybe there are non-standard systems out there?)
- the * sizeof(char) in malloc() is pointless - even if we want to support
systems where sizeof(char) != 1, current code is wrong as from by reading
of manpage, vsnprintf() takes buffer length in bytes but returns chars,
so the 'blen' must be bytes anyway and the sizeof(char) must be in line:
blen = bchar + 1;
The function seems to be essentially same since 7.2 so the patch should
apply to all branches. If you prefer you can apply cleanups to HEAD only.
--
marko
Attachment
"Marko Kreen" <markokr@gmail.com> writes:
> Following function crashes plpython on x86-64 / gcc 4.1.2 / debian 4.0:
> CREATE FUNCTION crashme(str_len integer)
> RETURNS text AS $$
> raise Exception("X" * str_len)
> $$ LANGUAGE plpythonu;
> SELECT crashme(1000);
> Problem turns out to be va_list handling in PLy_vprintf() which
> uses same va_list repeatedly. Fix is to va_copy to temp variable.
This patch isn't acceptable because va_copy() isn't portable.
I'm kinda wondering why PLy_printf and the functions after it even
exist. They look like rather poorly done reimplementations of
functionality that exists elsewhere in the backend (eg, stringinfo.c).
In particular, why malloc and not palloc?
regards, tom lane
Tom Lane escribió:
> "Marko Kreen" <markokr@gmail.com> writes:
> > Following function crashes plpython on x86-64 / gcc 4.1.2 / debian 4.0:
> > CREATE FUNCTION crashme(str_len integer)
> > RETURNS text AS $$
> > raise Exception("X" * str_len)
> > $$ LANGUAGE plpythonu;
>
> > SELECT crashme(1000);
>
> > Problem turns out to be va_list handling in PLy_vprintf() which
> > uses same va_list repeatedly. Fix is to va_copy to temp variable.
>
> This patch isn't acceptable because va_copy() isn't portable.
>
> I'm kinda wondering why PLy_printf and the functions after it even
> exist. They look like rather poorly done reimplementations of
> functionality that exists elsewhere in the backend (eg, stringinfo.c).
> In particular, why malloc and not palloc?
See attached patch.
I didn't bother to change the PLy_malloc and friends because I think
that would be too much change for 8.3. PLy_realloc is gone though
because there are no callers left after this patch.
--
Alvaro Herrera http://www.flickr.com/photos/alvherre/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)
Attachment
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Tom Lane escribi�:
>> This patch isn't acceptable because va_copy() isn't portable.
>>
>> I'm kinda wondering why PLy_printf and the functions after it even
>> exist. They look like rather poorly done reimplementations of
>> functionality that exists elsewhere in the backend (eg, stringinfo.c).
>> In particular, why malloc and not palloc?
> See attached patch.
> I didn't bother to change the PLy_malloc and friends because I think
> that would be too much change for 8.3. PLy_realloc is gone though
> because there are no callers left after this patch.
Yeah, that is about what I was thinking too. Are you set up to
back-patch this as far as 7.3? If so, please apply.
regards, tom lane
Tom Lane escribió: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > Tom Lane escribi�: > >> This patch isn't acceptable because va_copy() isn't portable. > >> > >> I'm kinda wondering why PLy_printf and the functions after it even > >> exist. They look like rather poorly done reimplementations of > >> functionality that exists elsewhere in the backend (eg, stringinfo.c). > >> In particular, why malloc and not palloc? > > > See attached patch. > > > I didn't bother to change the PLy_malloc and friends because I think > > that would be too much change for 8.3. PLy_realloc is gone though > > because there are no callers left after this patch. > > Yeah, that is about what I was thinking too. Are you set up to > back-patch this as far as 7.3? If so, please apply. Yeah, I think I'm done with the patch, but the regression tests do not work from 8.0 back in VPATH builds it seems, so I'll have to rebuild them to check. One problem here is that 7.3 does not have appendStringInfoVA. From 7.4 onwards there is no problem. Should I backport the code from 7.4? (On a first try to do it I introduced a nasty bug, so it's not as easy as a cut'n paste). -- Alvaro Herrera http://www.advogato.org/person/alvherre "Postgres is bloatware by design: it was built to house PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)
Alvaro Herrera escribió: > One problem here is that 7.3 does not have appendStringInfoVA. From 7.4 > onwards there is no problem. Should I backport the code from 7.4? (On > a first try to do it I introduced a nasty bug, so it's not as easy as > a cut'n paste). No nasty bug anywhere -- just that I neglected a initStringInfo call while manually backporting the patch. However, in testing I noticed this problem: alvherre=# select crashme(1); ERROR: plpython: Unable to create rexec.RExec instance exceptions.RuntimeError: This code is not secure in Python 2.2 and 2.3 There doesn't seem to be a way to make it work. What I'm going to do is commit the fix to just 7.4 onwards. -- Alvaro Herrera http://www.advogato.org/person/alvherre "We're here to devour each other alive" (Hobbes)
Marko Kreen escribió: > Problem turns out to be va_list handling in PLy_vprintf() which > uses same va_list repeatedly. Fix is to va_copy to temp variable. Marko, I just applied what I posted earlier today from 7.4 to 8.2 and HEAD. I assume you had a more complex test case; please have a try with the code in CVS and let me know. (If you are really interested in a fix for 7.3, let me know too ... I couldn't even get plpython to run a trivial function due to RExec issues. I didn't try 7.2). -- Alvaro Herrera Developer, http://www.PostgreSQL.org/ "Es filósofo el que disfruta con los enigmas" (G. Coli)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> What I'm going to do is commit the fix to just 7.4 onwards.
Fair enough. 7.3 is on life support anyway --- I'm not in favor of
heroic efforts to port patches that far back.
regards, tom lane
On 11/23/07, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Marko Kreen escribió: > > Problem turns out to be va_list handling in PLy_vprintf() which > > uses same va_list repeatedly. Fix is to va_copy to temp variable. > > Marko, I just applied what I posted earlier today from 7.4 to 8.2 and > HEAD. I assume you had a more complex test case; please have a try with > the code in CVS and let me know. Thanks, I'll test it. > (If you are really interested in a fix for 7.3, let me know too ... I > couldn't even get plpython to run a trivial function due to RExec > issues. I didn't try 7.2). RExec seems to hint that 7.3 can work only with python 2.2 and below. What version did you try it on? But no, I don't personally care about 7.3... -- marko
On 11/22/07, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > One problem here is that 7.3 does not have appendStringInfoVA. From 7.4 > onwards there is no problem. Should I backport the code from 7.4? (On > a first try to do it I introduced a nasty bug, so it's not as easy as > a cut'n paste). Just a note - appendStringInfoVA should take *nprinted argument. Can this be added in 8.4? Should make usage saner. -- marko
"Marko Kreen" <markokr@gmail.com> writes:
> Just a note - appendStringInfoVA should take *nprinted argument.
Why? I can't imagine any real use for it. If you're thinking that
it could provide a guide as to what to resize the buffer to, think
again.
regards, tom lane
On 11/23/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Marko Kreen" <markokr@gmail.com> writes: > > Just a note - appendStringInfoVA should take *nprinted argument. > > Why? I can't imagine any real use for it. If you're thinking that > it could provide a guide as to what to resize the buffer to, think > again. It seems perfectly appropriate: If the output was truncated due to this limit then the return value is the number of characters (not including the trailing '\0') which would have been written to the final string if enough space had been available. What problem do you see? Note that I don't want to fix unnecessary memory usage with that but several reallocs in case some args are large. -- marko
"Marko Kreen" <markokr@gmail.com> writes:
> On 11/23/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Why? I can't imagine any real use for it. If you're thinking that
>> it could provide a guide as to what to resize the buffer to, think
>> again.
> If the output was truncated due to this limit then the return
> value is the number of characters (not including the trailing
> '\0') which would have been written to the final string if
> enough space had been available.
> What problem do you see?
The problem is that you are quoting from some particular system's
manual, and not any kind of standard ... much less any standard that
every platform we support follows.
The real-world situation is that we are lucky to be able to tell
vsnprintf success from failure at all :-(
regards, tom lane
On 11/23/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Marko Kreen" <markokr@gmail.com> writes: > > On 11/23/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Why? I can't imagine any real use for it. If you're thinking that > >> it could provide a guide as to what to resize the buffer to, think > >> again. > > > If the output was truncated due to this limit then the return > > value is the number of characters (not including the trailing > > '\0') which would have been written to the final string if > > enough space had been available. > > > What problem do you see? > > The problem is that you are quoting from some particular system's > manual, and not any kind of standard ... much less any standard that > every platform we support follows. > > The real-world situation is that we are lucky to be able to tell > vsnprintf success from failure at all :-( Ah, ok. I just saw the result used inside the function, so I thought it is standard enough. Actually, the meaning could be changed to *needmore and compensated inside function: *needmore = (nprinted < buf->maxlen) ? buf->maxlen : nprinted + 1; Then it would not matter if libc is conforming or not. -- marko
Marko Kreen escribió: > On 11/23/07, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > (If you are really interested in a fix for 7.3, let me know too ... I > > couldn't even get plpython to run a trivial function due to RExec > > issues. I didn't try 7.2). > > RExec seems to hint that 7.3 can work only with python 2.2 > and below. What version did you try it on? I have 2.4.4 here. > But no, I don't personally care about 7.3... I must admit I was a bit annoyed at modifying 7.4's patch to fit and then finding out that I couldn't test it. -- Alvaro Herrera http://www.advogato.org/person/alvherre "If it wasn't for my companion, I believe I'd be having the time of my life" (John Dunbar)
"Marko Kreen" <markokr@gmail.com> writes:
> Actually, the meaning could be changed to *needmore
> and compensated inside function:
How's that different from the existing function result?
regards, tom lane