[python] [patch] PR python/13331

[python] [patch] PR python/13331

[Phil Muldoon]

David Malcolm's GCC plug-in Python reference checker:

https://fedorahosted.org/gcc-python-plugin/

Found a path in our code that could result in a segfault.  We do not
sanity check the result PyTuple_New and cascade those failures later
when we try to reference the bogus tuple.  This one was a little
trickier to fix, as I had to adjust the callers to expect the failure.
For some reason, the diff of this patch is ugly, but the summary is as
follows:

- Return NULL if PyTuple_New fails.
- In the caller, check if args is NULL.  If it is, do not call the
  convenience function, and skip onto the exception -> error converter.

OK?

Cheers,

Phil

--

2011-10-24  Phil Muldoon  <pmuldoon@redhat.com>

        PR python/13331

	* python/py-function.c (fnpy_call): Check 'args' is not NULL.
	(convert_values_to_python): Return on Python tuple allocation
	failure.

--

Index: python/py-function.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-function.c,v
retrieving revision 1.10
diff -u -r1.10 py-function.c
--- python/py-function.c	5 Aug 2011 14:24:10 -0000	1.10
+++ python/py-function.c	24 Oct 2011 13:15:38 -0000
@@ -38,6 +38,9 @@
 {
   int i;
   PyObject *result = PyTuple_New (argc);
+  
+  if (! result)
+    return NULL;
 
   for (i = 0; i < argc; ++i)
     {
@@ -59,24 +62,35 @@
 	   void *cookie, int argc, struct value **argv)
 {
   struct value *value = NULL;
-  PyObject *result, *callable, *args;
+  /* 'result' must be set to NULL, this initially indicates whether
+     the function was called, or not.  */
+  PyObject *result = NULL;
+  PyObject *callable, *args;
   struct cleanup *cleanup;
 
   cleanup = ensure_python_env (gdbarch, language);
 
   args = convert_values_to_python (argc, argv);
+  /* convert_values_to_python can return NULL on error.  If we
+     encounter this, do not call the function, but allow the Python ->
+     error code conversion below to deal with the Python exception.
+     Note, that this is different if the function simply does not
+     have arguments.  */
 
-  callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
-  if (! callable)
+  if (args)
     {
+      callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
+      if (! callable)
+	{
+	  Py_DECREF (args);
+	  error (_("No method named 'invoke' in object."));
+	}
+
+      result = PyObject_Call (callable, args, NULL);
+      Py_DECREF (callable);
       Py_DECREF (args);
-      error (_("No method named 'invoke' in object."));
     }
 
-  result = PyObject_Call (callable, args, NULL);
-  Py_DECREF (callable);
-  Py_DECREF (args);
-
   if (!result)
     {
       PyObject *ptype, *pvalue, *ptraceback;

Re: [python] [patch] PR python/13331

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> 2011-10-24  Phil Muldoon  <pmuldoon@redhat.com>
Phil>         PR python/13331
Phil> 	* python/py-function.c (fnpy_call): Check 'args' is not NULL.
Phil> 	(convert_values_to_python): Return on Python tuple allocation
Phil> 	failure.

Phil> +  if (! result)
Phil> +    return NULL;

I think the function should have a single return convention.
One easy way to do that would be to remove the 'error' call and have it
return NULL when value_to_value_object fails.

Tom

Re: [python] [patch] PR python/13331

[Paul Koning]

> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Tom Tromey
> Subject: Re: [python] [patch] PR python/13331
> 
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> 
> Phil> 2011-10-24  Phil Muldoon  <pmuldoon@redhat.com>
> Phil>         PR python/13331
> Phil> 	* python/py-function.c (fnpy_call): Check 'args' is not NULL.
> Phil> 	(convert_values_to_python): Return on Python tuple allocation
> Phil> 	failure.
> 
> Phil> +  if (! result)
> Phil> +    return NULL;
> 
> I think the function should have a single return convention.
> One easy way to do that would be to remove the 'error' call and have it return NULL when value_to_value_object fails.

I think the patch is not quite right because it goes against the Python rule for exception handling.  A NULL function result is the normal way to indicate an error, but associated with returning NULL is that in the spot where the error is first detected you set the exception status.  And in addition, anywhere else NULL is propagated (with any necessary cleanup) but the exception status is *not* overwritten.

> ...
> Index: python/py-function.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/python/py-function.c,v
> retrieving revision 1.10
> diff -u -r1.10 py-function.c
> --- python/py-function.c	5 Aug 2011 14:24:10 -0000	1.10
> +++ python/py-function.c	24 Oct 2011 13:15:38 -0000
> @@ -38,6 +38,9 @@
> {
>   int i;
>   PyObject *result = PyTuple_New (argc);
> +  
> +  if (! result)
> +    return NULL;
> 
>   for (i = 0; i < argc; ++i)
>     {

In this loop there is an example of what I mentioned: if value_to_value_object returns NULL, exception status has been set, and that should not be overwritten here.  On the other hand, there is missing cleanup: any objects added to the tuple before the failing call need to have a DECREF done on them.


> @@ -59,24 +62,35 @@
> 	   void *cookie, int argc, struct value **argv)  {
>   struct value *value = NULL;
> -  PyObject *result, *callable, *args;
> +  /* 'result' must be set to NULL, this initially indicates whether
> +     the function was called, or not.  */  PyObject *result = NULL;  
> + PyObject *callable, *args;
>   struct cleanup *cleanup;
> 
>   cleanup = ensure_python_env (gdbarch, language);
> 
>   args = convert_values_to_python (argc, argv);
> +  /* convert_values_to_python can return NULL on error.  If we
> +     encounter this, do not call the function, but allow the Python ->
> +     error code conversion below to deal with the Python exception.
> +     Note, that this is different if the function simply does not
> +     have arguments.  */
> 
> -  callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
> -  if (! callable)
> +  if (args)
>     {
> +      callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
> +      if (! callable)
> +	{
> +	  Py_DECREF (args);
> +	  error (_("No method named 'invoke' in object."));

That should just be a cleanup and return NULL -- the PyObject_GetAttrString has set the exception.

	paul

Re: [python] [patch] PR python/13331

[Tom Tromey]

>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:

Paul> In this loop there is an example of what I mentioned: if
Paul> value_to_value_object returns NULL, exception status has been set, and
Paul> that should not be overwritten here.  On the other hand, there is
Paul> missing cleanup: any objects added to the tuple before the failing
Paul> call need to have a DECREF done on them.

Won't decref'ing the tuple do this for us?
If not then I think there are a lot of similar bugs elsewhere.
But .. how could it not?  That would be weird.

Paul> That should just be a cleanup and return NULL -- the
Paul> PyObject_GetAttrString has set the exception.

This (fnpy_call) is a "gdb-facing" function, so it has to use error.
It doesn't (and can't) follow the Python conventions.

Tom

Re: [python] [patch] PR python/13331

[Paul Koning]

On Oct 24, 2011, at 11:38 AM, Tom Tromey wrote:

>>>>>> "Paul" == Paul Koning <paulkoning@comcast.net> writes:
> 
> Paul> In this loop there is an example of what I mentioned: if
> Paul> value_to_value_object returns NULL, exception status has been set, and
> Paul> that should not be overwritten here.  On the other hand, there is
> Paul> missing cleanup: any objects added to the tuple before the failing
> Paul> call need to have a DECREF done on them.
> 
> Won't decref'ing the tuple do this for us?
> If not then I think there are a lot of similar bugs elsewhere.
> But .. how could it not?  That would be weird.

Oops.  Yes, you're right.  I was worried about the case where you have the tuple half set up.  But that case is handled.

Sorry for the confusion.

	paul

Re: [python] [patch] PR python/13331

[Phil Muldoon]

Paul Koning <paulkoning@comcast.net> writes:

>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Tom Tromey
>> Subject: Re: [python] [patch] PR python/13331
>> 
>>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>> 
>> Phil> 2011-10-24  Phil Muldoon  <pmuldoon@redhat.com>
>> Phil>         PR python/13331
>> Phil> 	* python/py-function.c (fnpy_call): Check 'args' is not NULL.
>> Phil> 	(convert_values_to_python): Return on Python tuple allocation
>> Phil> 	failure.
>> 
>> Phil> +  if (! result)
>> Phil> +    return NULL;
>> 
>> I think the function should have a single return convention.
>> One easy way to do that would be to remove the 'error' call and have it return NULL when value_to_value_object fails.
>
> I think the patch is not quite right because it goes against the
> Python rule for exception handling.  A NULL function result is the
> normal way to indicate an error, but associated with returning NULL is
> that in the spot where the error is first detected you set the
> exception status.  

The exception is set by the PyTuple_New.  We purely propagate it, until
it is handled.

> And in addition, anywhere else NULL is propagated (with any necessary
> cleanup) but the exception status is *not* overwritten.

In this patch, and in this functionality it is not.  The Python
exception in the caller gets converted to a GDB error and the exception
cleared.



>> ...
>> Index: python/py-function.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/python/py-function.c,v
>> retrieving revision 1.10
>> diff -u -r1.10 py-function.c
>> --- python/py-function.c	5 Aug 2011 14:24:10 -0000	1.10
>> +++ python/py-function.c	24 Oct 2011 13:15:38 -0000
>> @@ -38,6 +38,9 @@
>> {
>>   int i;
>>   PyObject *result = PyTuple_New (argc);
>> +  
>> +  if (! result)
>> +    return NULL;
>> 
>>   for (i = 0; i < argc; ++i)
>>     {
>
> In this loop there is an example of what I mentioned: if
> value_to_value_object returns NULL, exception status has been set, and
> that should not be overwritten here.  On the other hand, there is
> missing cleanup: any objects added to the tuple before the failing
> call need to have a DECREF done on them.

I'm not sure what you mean by this.  If result fails, the args check in
the calling function will fail, the exception converted to a gdb_error,
the exception bits cleared and the call will fail. The loop will not be
executed.  It is also my understanding that PyTuple_SetItem steals a
reference to the object being added.  In this case, there is only one
reference to the object, so the logic here works.  When the tuple is
garbage collected, the reference count for each object will
automatically be decremented and therefore garbage collected too.

>
>
>> @@ -59,24 +62,35 @@
>> 	   void *cookie, int argc, struct value **argv)  {
>>   struct value *value = NULL;
>> -  PyObject *result, *callable, *args;
>> +  /* 'result' must be set to NULL, this initially indicates whether
>> +     the function was called, or not.  */  PyObject *result = NULL;  
>> + PyObject *callable, *args;
>>   struct cleanup *cleanup;
>> 
>>   cleanup = ensure_python_env (gdbarch, language);
>> 
>>   args = convert_values_to_python (argc, argv);
>> +  /* convert_values_to_python can return NULL on error.  If we
>> +     encounter this, do not call the function, but allow the Python ->
>> +     error code conversion below to deal with the Python exception.
>> +     Note, that this is different if the function simply does not
>> +     have arguments.  */
>> 
>> -  callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
>> -  if (! callable)
>> +  if (args)
>>     {
>> +      callable = PyObject_GetAttrString ((PyObject *) cookie, "invoke");
>> +      if (! callable)
>> +	{
>> +	  Py_DECREF (args);
>> +	  error (_("No method named 'invoke' in object."));
>
> That should just be a cleanup and return NULL -- the
> PyObject_GetAttrString has set the exception.

Well yes and no, it is seemly expedient here to short-circuit the
process and convert it to a gdb error directly.  The error is more
informative than the error from GetAttrString -- that is there is no
"invoke" function.  This is something we (as in GDB), want to enforce in
our API

Cheers,

Phil

Re: [python] [patch] PR python/13331

[Paul Koning]

On Oct 24, 2011, at 11:51 AM, Phil Muldoon wrote:

> Paul Koning <paulkoning@comcast.net> writes:
> 
>>> ... On the other hand, there is
>> missing cleanup: any objects added to the tuple before the failing
>> call need to have a DECREF done on them.
> 
> I'm not sure what you mean by this.  If result fails, the args check in
> the calling function will fail, the exception converted to a gdb_error,
> the exception bits cleared and the call will fail. The loop will not be
> executed.  It is also my understanding that PyTuple_SetItem steals a
> reference to the object being added.  In this case, there is only one
> reference to the object, so the logic here works.  When the tuple is
> garbage collected, the reference count for each object will
> automatically be decremented and therefore garbage collected too.

Yes, I got confused.  Sorry.

> ...
>> 
>> That should just be a cleanup and return NULL -- the
>> PyObject_GetAttrString has set the exception.
> 
> Well yes and no, it is seemly expedient here to short-circuit the
> process and convert it to a gdb error directly.  The error is more
> informative than the error from GetAttrString -- that is there is no
> "invoke" function.  This is something we (as in GDB), want to enforce in
> our API

I see what you mean, that makes sense.  Keep the original exception unless there is a reason to replace it -- for example to add more detail or make it more descriptive.

	paul

Re: [python] [patch] PR python/13331

[Phil Muldoon]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
>
> Phil> 2011-10-24  Phil Muldoon  <pmuldoon@redhat.com>
> Phil>         PR python/13331
> Phil> 	* python/py-function.c (fnpy_call): Check 'args' is not NULL.
> Phil> 	(convert_values_to_python): Return on Python tuple allocation
> Phil> 	failure.
>
> Phil> +  if (! result)
> Phil> +    return NULL;
>
> I think the function should have a single return convention.
> One easy way to do that would be to remove the 'error' call and have it
> return NULL when value_to_value_object fails.

Committed with requested change (return NULL on value conversion
error).

Cheers,

Phil