[patch] fix for crash in python pretty-printer

[patch] fix for crash in python pretty-printer

[Paul Pluzhnikov]

Greetings,

This is rather on the obvious side.
While debugging a buggy pretty-printer, GDB crashed on me:

Program received signal SIGSEGV, Segmentation fault.
0x000000000045db13 in do_my_cleanups (pmy_chain=0xa7c020, old_chain=0x1e65a10) at ../../gdb/utils.c:390
390           *pmy_chain = ptr->next;   /* Do this first incase recursion */

  ptr is NULL at that point.

(top) bt
#0  0x000000000045db13 in do_my_cleanups (pmy_chain=0xa7c020, old_chain=0x1e65a10) at ../../gdb/utils.c:390
#1  0x000000000045dadf in do_cleanups (...) at ../../gdb/utils.c:374
#2  0x00000000004d684b in apply_val_pretty_printer (...) at ../../gdb/python/python-prettyprint.c:526
#3  0x000000000051286a in value_print (...) at ../../gdb/valprint.c:388
#4  0x0000000000514f53 in print_formatted (...) at ../../gdb/printcmd.c:316
...

Attached is a fix (applies both to mainline and archer-tromey-python).
Tested on Linux/x86_64 with no regressions.

P.S. Is there an "easy" way to find such mis-uses of TRY_CATCH?
This one took me 2 hours to find :-(

Thanks,
--
Paul Pluzhnikov

2009-07-07  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* python/python-value.c (valpy_getitem): Don't return from TRY_CATCH.

diff --git a/gdb/python/python-value.c b/gdb/python/python-value.c
index 8c85ef6..489b65b 100644
--- a/gdb/python/python-value.c
+++ b/gdb/python/python-value.c
@@ -290,8 +290,7 @@ valpy_getitem (PyObject *self, PyObject *key)
 {
   value_object *self_value = (value_object *) self;
   char *field = NULL;
-  struct value *idx = NULL;
-  struct value *res_val = NULL;	  /* Initialize to appease gcc warning.  */
+  struct value *res_val = NULL;
   volatile struct gdb_exception except;
 
   if (gdbpy_is_string (key))
@@ -313,12 +312,17 @@ valpy_getitem (PyObject *self, PyObject *key)
 	     value code throw an exception if the index has an invalid
 	     type.  */
 	  struct value *idx = convert_value_from_python (key);
-	  if (idx == NULL)
-	    return NULL;
-
-	  res_val = value_subscript (tmp, value_as_long (idx));
+	  if (idx != NULL)
+	    res_val = value_subscript (tmp, value_as_long (idx));
 	}
     }
+
+  if (res_val == NULL)
+    {
+      gdb_assert (field == NULL);
+      return NULL;
+    }
+
   if (field)
     xfree (field);
   GDB_PY_HANDLE_EXCEPTION (except);

Re: [patch] fix for crash in python pretty-printer

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Attached is a fix (applies both to mainline and archer-tromey-python).
Paul> Tested on Linux/x86_64 with no regressions.

This is ok.  Thanks.

Paul> P.S. Is there an "easy" way to find such mis-uses of TRY_CATCH?
Paul> This one took me 2 hours to find :-(

Not that I know of :(

You could try coccinelle, I suppose.  I've run it for other purposes
on gdb, though with mixed results.

Tom

Re: [patch] fix for crash in python pretty-printer

[Paul Pluzhnikov]

On Tue, Jul 7, 2009 at 11:23 AM, Tom Tromey<tromey@redhat.com> wrote:

> This is ok.  Thanks.

Committed to the mainline.

> You could try coccinelle, I suppose.  I've run it for other purposes
> on gdb, though with mixed results.

For reference, here is an intro: http://lwn.net/Articles/315686/

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] fix for crash in python pretty-printer

[Paul Pluzhnikov]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On Tue, Jul 7, 2009 at 12:50 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> Committed to the mainline.

It turns out I didn't follow what happens when value_struct_elt() raises
error correctly: when it does, res_val will be left as NULL, and that will
trigger gdb_assert in my previous patch.

Here is a fix. Sorry for the noise :-(

-- 
Paul Pluzhnikov

2009-07-07  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* python/python-value.c (valpy_getitem): Remove incorrect assert.

[-- Attachment #2: gdb-pretty-printer-crash-20090707-2.txt --]
[-- Type: text/plain, Size: 682 bytes --]

Index: python/python-value.c
===================================================================
RCS file: /cvs/src/src/gdb/python/python-value.c,v
retrieving revision 1.22
diff -u -p -u -r1.22 python-value.c
--- python/python-value.c	7 Jul 2009 19:36:09 -0000	1.22
+++ python/python-value.c	7 Jul 2009 20:32:27 -0000
@@ -294,17 +294,11 @@ valpy_getitem (PyObject *self, PyObject 
 	}
     }
 
-  if (res_val == NULL)
-    {
-      gdb_assert (field == NULL);
-      return NULL;
-    }
-
   if (field)
     xfree (field);
   GDB_PY_HANDLE_EXCEPTION (except);
 
-  return value_to_value_object (res_val);
+  return res_val ? value_to_value_object (res_val) : NULL;
 }
 
 static int

Re: [patch] fix for crash in python pretty-printer

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> It turns out I didn't follow what happens when value_struct_elt() raises
Paul> error correctly: when it does, res_val will be left as NULL, and that will
Paul> trigger gdb_assert in my previous patch.

I missed that too.

Paul> Here is a fix. Sorry for the noise :-(

No big deal.  This is ok if you haven't committed it already.

Paul>    if (field)
Paul>      xfree (field);

Jim Meyering has been going around removing these redundant 'if's.
This is something I'm *sure* coccinelle can do :-)

BTW, I neglected to mention upthread -- Taras has got dehydra and
treehydra working for C.  So, you could probably write a TRY_CATCH
checker in javascript without too much trouble.

Tom

Re: [patch] fix for crash in python pretty-printer

[Paul Pluzhnikov]

On Tue, Jul 7, 2009 at 1:54 PM, Tom Tromey<tromey@redhat.com> wrote:

> This is ok if you haven't committed it already.

Thanks, now committed.

> Jim Meyering has been going around removing these redundant 'if's.

I killed that one :)

-- 
Paul Pluzhnikov