PL/Python error checking

This patch addresses the problem mentioned in the "process crash
when a plpython function returns unicode" thread:

http://archives.postgresql.org/pgsql-bugs/2005-06/msg00105.php

In several places PL/Python was calling PyObject_Str() and then
PyString_AsString() without checking if the former had returned
NULL to indicate an error. PyString_AsString() doesn't expect a
NULL argument, so passing one causes a segmentation fault. This
patch adds checks for NULL and raises errors via PLy_elog(), which
prints details of the underlying Python exception. The patch also
adds regression tests for these checks. All tests pass on my
Solaris 9 box running HEAD and Python 2.4.1.

In one place the patch doesn't call PLy_elog() because that could
cause infinite recursion; see the comment I added. I'm not sure
how to test that particular case or whether it's even possible to
get an error there: the value that the code should check is the
Python exception type, so I wonder if a NULL value "shouldn't
happen." This patch converts NULL to "Unknown Exception" but I
wonder if an Assert() would be appropriate.

The patch is against HEAD but the same changes should be applied
to earlier versions because they have the same problem. The patch
might not apply cleanly against earlier versions -- will the committer
take care of little differences or should I submit different versions
of the patch?

--
Michael Fuhr
http://www.fuhr.org/~mfuhr/

Patch applied. Thanks.

---------------------------------------------------------------------------

Michael Fuhr wrote:
> This patch addresses the problem mentioned in the "process crash
> when a plpython function returns unicode" thread:
>
> http://archives.postgresql.org/pgsql-bugs/2005-06/msg00105.php
>
> In several places PL/Python was calling PyObject_Str() and then
> PyString_AsString() without checking if the former had returned
> NULL to indicate an error. PyString_AsString() doesn't expect a
> NULL argument, so passing one causes a segmentation fault. This
> patch adds checks for NULL and raises errors via PLy_elog(), which
> prints details of the underlying Python exception. The patch also
> adds regression tests for these checks. All tests pass on my
> Solaris 9 box running HEAD and Python 2.4.1.
>
> In one place the patch doesn't call PLy_elog() because that could
> cause infinite recursion; see the comment I added. I'm not sure
> how to test that particular case or whether it's even possible to
> get an error there: the value that the code should check is the
> Python exception type, so I wonder if a NULL value "shouldn't
> happen." This patch converts NULL to "Unknown Exception" but I
> wonder if an Assert() would be appropriate.
>
> The patch is against HEAD but the same changes should be applied
> to earlier versions because they have the same problem. The patch
> might not apply cleanly against earlier versions -- will the committer
> take care of little differences or should I submit different versions
> of the patch?
>
> --
> Michael Fuhr
> http://www.fuhr.org/~mfuhr/

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Michael Fuhr wrote:
> This patch addresses the problem mentioned in the "process crash
> when a plpython function returns unicode" thread:
>
> http://archives.postgresql.org/pgsql-bugs/2005-06/msg00105.php
>
> In several places PL/Python was calling PyObject_Str() and then
> PyString_AsString() without checking if the former had returned
> NULL to indicate an error. PyString_AsString() doesn't expect a
> NULL argument, so passing one causes a segmentation fault. This
> patch adds checks for NULL and raises errors via PLy_elog(), which
> prints details of the underlying Python exception. The patch also
> adds regression tests for these checks. All tests pass on my
> Solaris 9 box running HEAD and Python 2.4.1.
>
> In one place the patch doesn't call PLy_elog() because that could
> cause infinite recursion; see the comment I added. I'm not sure
> how to test that particular case or whether it's even possible to
> get an error there: the value that the code should check is the
> Python exception type, so I wonder if a NULL value "shouldn't
> happen." This patch converts NULL to "Unknown Exception" but I
> wonder if an Assert() would be appropriate.
>
> The patch is against HEAD but the same changes should be applied
> to earlier versions because they have the same problem. The patch
> might not apply cleanly against earlier versions -- will the committer
> take care of little differences or should I submit different versions
> of the patch?

I am unclear about backpatching this. We have to weigh the risks of
applying or not applying to 8.0.X. Comments?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Sun, Jul 10, 2005 at 12:58:24AM -0400, Bruce Momjian wrote:
> Michael Fuhr wrote:
> > The patch is against HEAD but the same changes should be applied
> > to earlier versions because they have the same problem. The patch
> > might not apply cleanly against earlier versions -- will the committer
> > take care of little differences or should I submit different versions
> > of the patch?
>
> I am unclear about backpatching this. We have to weigh the risks of
> applying or not applying to 8.0.X. Comments?

Since 7.4, PL/Python is only available as an untrusted language,
so only a database superuser could create an exploitable function.
However, it might be possible for an ordinary user to tickle the
bug by calling such a function and passing it certain data, either
as an argument or as table data. The code is buggy in any case:
PyObject_Str() is documented to return NULL on error, and
PyString_AsString() doesn't expect a NULL pointer so it segfaults
if passed one. Since the patch simply checks for that condition
and raises an error instead of calling a function that will segfault
and take down the backend, I can't think of what risk applying the
patch would have. The greater risk would seem to be in not applying
it.

--
Michael Fuhr
http://www.fuhr.org/~mfuhr/

On Mon, Jul 11, 2005 at 08:13:24PM -0600, Michael Fuhr wrote:
> On Sun, Jul 10, 2005 at 12:58:24AM -0400, Bruce Momjian wrote:
> > I am unclear about backpatching this. We have to weigh the risks of
> > applying or not applying to 8.0.X. Comments?
>
> Since 7.4, PL/Python is only available as an untrusted language,
> so only a database superuser could create an exploitable function.
> However, it might be possible for an ordinary user to tickle the
> bug by calling such a function and passing it certain data, either
> as an argument or as table data. The code is buggy in any case:
> PyObject_Str() is documented to return NULL on error, and
> PyString_AsString() doesn't expect a NULL pointer so it segfaults
> if passed one. Since the patch simply checks for that condition
> and raises an error instead of calling a function that will segfault
> and take down the backend, I can't think of what risk applying the
> patch would have. The greater risk would seem to be in not applying
> it.

I haven't seen this patch applied to other than HEAD. Since it
fixes a segmentation fault, should it be backpatched before the
new releases?

Here's the original patch submission:

http://archives.postgresql.org/pgsql-patches/2005-06/msg00519.php

--
Michael Fuhr

Michael Fuhr wrote:
> On Mon, Jul 11, 2005 at 08:13:24PM -0600, Michael Fuhr wrote:
> > On Sun, Jul 10, 2005 at 12:58:24AM -0400, Bruce Momjian wrote:
> > > I am unclear about backpatching this. We have to weigh the risks of
> > > applying or not applying to 8.0.X. Comments?
> >
> > Since 7.4, PL/Python is only available as an untrusted language,
> > so only a database superuser could create an exploitable function.
> > However, it might be possible for an ordinary user to tickle the
> > bug by calling such a function and passing it certain data, either
> > as an argument or as table data. The code is buggy in any case:
> > PyObject_Str() is documented to return NULL on error, and
> > PyString_AsString() doesn't expect a NULL pointer so it segfaults
> > if passed one. Since the patch simply checks for that condition
> > and raises an error instead of calling a function that will segfault
> > and take down the backend, I can't think of what risk applying the
> > patch would have. The greater risk would seem to be in not applying
> > it.
>
> I haven't seen this patch applied to other than HEAD. Since it
> fixes a segmentation fault, should it be backpatched before the
> new releases?
>
> Here's the original patch submission:
>
> http://archives.postgresql.org/pgsql-patches/2005-06/msg00519.php

I have backpatched this to 8.0.X. It did not apply cleanly to 7.4.X so
if you would like that version patched please submit a matching patch.
Thanks. (I don't trust myself to adjust the patch for 7.4.X.)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Fri, Sep 23, 2005 at 05:03:02PM -0400, Bruce Momjian wrote:
> Michael Fuhr wrote:
> > http://archives.postgresql.org/pgsql-patches/2005-06/msg00519.php
>
> I have backpatched this to 8.0.X. It did not apply cleanly to 7.4.X so
> if you would like that version patched please submit a matching patch.
> Thanks. (I don't trust myself to adjust the patch for 7.4.X.)

Here's a patch for 7.4. I had to s/PLy_curr_procedure/PLy_last_procedure/
because 7.4 uses the latter where 8.x uses the former.

How far back do you want to backpatch? 7.3? 7.2? The patch is
pretty simple so I could make patches for older versions if necessary.

--
Michael Fuhr

Michael Fuhr wrote:
> On Fri, Sep 23, 2005 at 05:03:02PM -0400, Bruce Momjian wrote:
> > Michael Fuhr wrote:
> > > http://archives.postgresql.org/pgsql-patches/2005-06/msg00519.php
> >
> > I have backpatched this to 8.0.X. It did not apply cleanly to 7.4.X so
> > if you would like that version patched please submit a matching patch.
> > Thanks. (I don't trust myself to adjust the patch for 7.4.X.)
>
> Here's a patch for 7.4. I had to s/PLy_curr_procedure/PLy_last_procedure/
> because 7.4 uses the latter where 8.x uses the former.
>
> How far back do you want to backpatch? 7.3? 7.2? The patch is
> pretty simple so I could make patches for older versions if necessary.

Applied to 7.4.X. I that that is as far back as we want to go.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073