diff mbox

[RFA,1/2] Fix some error-handling bugs in python frame filters

Message ID 87y4105g42.fsf@tromey.com
State New
Headers show

Commit Message

Tom Tromey Nov. 3, 2016, 10:54 p.m. UTC
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I've included a test case for the first issue.

I did a -m32 build here and have fixed up the test case.
Here's the new patch.  This is ready to review now.

Tom

commit 981628a4af5f82a12351b9764437927d3a8c8169
Author: Tom Tromey <tom@tromey.com>
Date:   Mon Oct 31 11:10:35 2016 -0600

    Fix some error-handling bugs in python frame filters
    
    While writing a Python frame filter, I found a few bugs in the current
    frame filter code.  In particular:
    
    * One spot converts a Python long to a CORE_ADDR using PyLong_AsLong.
      However, this can fail on overflow.  I changed this to use
      get_addr_from_python.
    
    * Another spot is doing the same but with PyLong_AsUnsignedLongLong; I
      changed this as well just for consistency.
    
    * Converting line numbers can print "-1" if conversion from long
      fails.  This isn't fatal but just a bit ugly.
    
    I've included a test case for the first issue.  The line number one
    didn't seem important enough to bother with.
    
    2016-10-31  Tom Tromey  <tom@tromey.com>
    
    	* python/py-framefilter.c (py_print_frame): Use
    	get_addr_from_python.  Check for errors when getting line number.
    
    2016-10-31  Tom Tromey  <tom@tromey.com>
    
    	* gdb.python/py-framefilter.py (ElidingFrameDecorator.address):
    	New method.

Comments

Pedro Alves Nov. 8, 2016, 1:07 p.m. UTC | #1
On 11/03/2016 10:54 PM, Tom Tromey wrote:

>     Fix some error-handling bugs in python frame filters
>     
>     While writing a Python frame filter, I found a few bugs in the current
>     frame filter code.  In particular:
>     
>     * One spot converts a Python long to a CORE_ADDR using PyLong_AsLong.
>       However, this can fail on overflow.  I changed this to use
>       get_addr_from_python.
>     
>     * Another spot is doing the same but with PyLong_AsUnsignedLongLong; I
>       changed this as well just for consistency.

Ah, I wondered about this at:

 https://sourceware.org/ml/gdb-patches/2016-10/msg00041.html

Your patch obsoletes Matthias' then.

>     
>     * Converting line numbers can print "-1" if conversion from long
>       fails.  This isn't fatal but just a bit ugly.
>     
>     I've included a test case for the first issue.  The line number one
>     didn't seem important enough to bother with.
>     
>     2016-10-31  Tom Tromey  <tom@tromey.com>
>     
>     	* python/py-framefilter.c (py_print_frame): Use
>     	get_addr_from_python.  Check for errors when getting line number.
>     
>     2016-10-31  Tom Tromey  <tom@tromey.com>
>     
>     	* gdb.python/py-framefilter.py (ElidingFrameDecorator.address):
>     	New method.
> 

Great, LGTM.

Thanks,
Pedro Alves
Jan Kratochvil Jan. 11, 2017, 4:56 p.m. UTC | #2
On Tue, 08 Nov 2016 14:07:11 +0100, Pedro Alves wrote:
> On 11/03/2016 10:54 PM, Tom Tromey wrote:
> >     
> >     * Converting line numbers can print "-1" if conversion from long
> >       fails.  This isn't fatal but just a bit ugly.
> >     
> >     I've included a test case for the first issue.  The line number one
> >     didn't seem important enough to bother with.
> >     
> >     2016-10-31  Tom Tromey  <tom@tromey.com>
> >     
> >     	* python/py-framefilter.c (py_print_frame): Use
> >     	get_addr_from_python.  Check for errors when getting line number.
> >     
> >     2016-10-31  Tom Tromey  <tom@tromey.com>
> >     
> >     	* gdb.python/py-framefilter.py (ElidingFrameDecorator.address):
> >     	New method.
> > 
> 
> Great, LGTM.

https://bugzilla.redhat.com/show_bug.cgi?id=1411094

asked for its backport for Fedora GDB 7.12.  So I expect it would be good even
for FSF GDB 7.12.1.


Jan
Pedro Alves Jan. 12, 2017, 4:20 p.m. UTC | #3
On 01/11/2017 04:56 PM, Jan Kratochvil wrote:

> https://bugzilla.redhat.com/show_bug.cgi?id=1411094
> 
> asked for its backport for Fedora GDB 7.12.  So I expect it would be good even
> for FSF GDB 7.12.1.

OK.

Thanks,
Pedro Alves
Jan Kratochvil Jan. 12, 2017, 5:04 p.m. UTC | #4
On Thu, 12 Jan 2017 17:20:59 +0100, Pedro Alves wrote:
> On 01/11/2017 04:56 PM, Jan Kratochvil wrote:
> 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1411094
> > 
> > asked for its backport for Fedora GDB 7.12.  So I expect it would be good even
> > for FSF GDB 7.12.1.
> 
> OK.

Checked in 7.12 branch:
	df1fe27b672d7aa054eaad8b51f86302f7e46c65


Thanks,
Jan
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1fd85ce..98fcd21 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@ 
+2016-10-31  Tom Tromey  <tom@tromey.com>
+
+	* python/py-framefilter.c (py_print_frame): Use
+	get_addr_from_python.  Check for errors when getting line number.
+
 2016-11-03  Yao Qi  <yao.qi@linaro.org>
 
 	* Makefile.in (.y.c): Replace YY_NULL with YY_NULLPTR.
diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
index 6692ac5..4c7757c 100644
--- a/gdb/python/py-framefilter.c
+++ b/gdb/python/py-framefilter.c
@@ -1116,7 +1116,13 @@  py_print_frame (PyObject *filter, int flags,
 
 	  if (paddr != Py_None)
 	    {
-	      address = PyLong_AsLong (paddr);
+	      if (get_addr_from_python (paddr, &address) < 0)
+		{
+		  Py_DECREF (paddr);
+		  do_cleanups (cleanup_stack);
+		  return EXT_LANG_BT_ERROR;
+		}
+
 	      has_addr = 1;
 	    }
 	  Py_DECREF (paddr);
@@ -1213,10 +1219,10 @@  py_print_frame (PyObject *filter, int flags,
 	    }
 	  else if (PyLong_Check (py_func))
 	    {
-	      CORE_ADDR addr = PyLong_AsUnsignedLongLong (py_func);
+	      CORE_ADDR addr;
 	      struct bound_minimal_symbol msymbol;
 
-	      if (PyErr_Occurred ())
+	      if (get_addr_from_python (py_func, &addr) < 0)
 		{
 		  do_cleanups (cleanup_stack);
 		  return EXT_LANG_BT_ERROR;
@@ -1340,6 +1346,12 @@  py_print_frame (PyObject *filter, int flags,
 	  if (py_line != Py_None)
 	    {
 	      line = PyLong_AsLong (py_line);
+	      if (PyErr_Occurred ())
+		{
+		  do_cleanups (cleanup_stack);
+		  return EXT_LANG_BT_ERROR;
+		}
+
 	      TRY
 		{
 		  ui_out_text (out, ":");
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 52038e3..d8466f1 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2016-10-31  Tom Tromey  <tom@tromey.com>
+
+	* gdb.python/py-framefilter.py (ElidingFrameDecorator.address):
+	New method.
+
 2016-10-28  Pedro Alves  <palves@redhat.com>
 
 	* gdb.base/maint.exp <maint info line-table w/o a file name>: Use
diff --git a/gdb/testsuite/gdb.python/py-framefilter.py b/gdb/testsuite/gdb.python/py-framefilter.py
index 8fdff84..2580911 100644
--- a/gdb/testsuite/gdb.python/py-framefilter.py
+++ b/gdb/testsuite/gdb.python/py-framefilter.py
@@ -92,6 +92,12 @@  class ElidingFrameDecorator(FrameDecorator):
     def elided(self):
         return iter(self.elided_frames)
 
+    def address (self):
+        # Regression test for an overflow in the python layer.
+        bitsize = 8 * gdb.lookup_type('void').pointer().sizeof
+        mask = (1 << bitsize) - 1
+        return 0xffffffffffffffff & mask
+
 class ElidingIterator:
     def __init__(self, ii):
         self.input_iterator = ii