Thread: 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/
Attachment
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@postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.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@candle.pha.pa.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@candle.pha.pa.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
Attachment
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@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073