From: Andrew Burgess <aburgess@redhat.com>
Date: Tue, 22 Apr 2025 20:31:02 +0000 (+0100)
Subject: gdb/python: stop using PyObject_IsInstance in py-disasm.c
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=dff7f6c72d05d8a378dc346e6900ba7c4eba759f;p=thirdparty%2Fbinutils-gdb.git

gdb/python: stop using PyObject_IsInstance in py-disasm.c

The PyObject_IsInstance function can return -1 for errors, 0 to
indicate false, and 1 to indicate true.

I noticed in python/py-disasm.c that we treat the result of
PyObject_IsInstance as a bool.  This means that if PyObject_IsInstance
returns -1, then this will be treated as true.  The consequence of
this is that we will invoke undefined behaviour by treating the result
from the _print_insn call as if it was a DisassemblerResult object,
even though PyObject_IsInstance raised an error, and the result might
not be of the required type.

I could fix this by taking the -1 result into account, however,
gdb.DisassemblerResult cannot be sub-classed, the type doesn't have
the Py_TPFLAGS_BASETYPE flag.  As such, we can switch to using
PyObject_TypeCheck instead, which only return 0 or 1, with no error
case.

I have also taken the opportunity to improve the error message emitted
if the result has the wrong type.  Better error message make debugging
issues easier.

I've added a test which exposes the problem when using
PyObject_IsInstance, and I've updated the existing test for the
improved error message.

Approved-By: Tom Tromey <tom@tromey.com>
---

diff --git a/gdb/python/py-disasm.c b/gdb/python/py-disasm.c
index 9ca8d225e7e..17064dc0c30 100644
--- a/gdb/python/py-disasm.c
+++ b/gdb/python/py-disasm.c
@@ -1311,12 +1311,13 @@ gdbpy_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
       return {};
     }
 
-  /* Check the result is a DisassemblerResult (or a sub-class).  */
-  if (!PyObject_IsInstance (result.get (),
-			    (PyObject *) &disasm_result_object_type))
+  /* Check the result is a DisassemblerResult.  */
+  if (!PyObject_TypeCheck (result.get (), &disasm_result_object_type))
     {
-      PyErr_SetString (PyExc_TypeError,
-		       _("Result is not a DisassemblerResult."));
+      PyErr_Format
+	(PyExc_TypeError,
+	 _("Result from Disassembler must be gdb.DisassemblerResult, not %s."),
+	 Py_TYPE (result.get ())->tp_name);
       gdbpy_print_stack ();
       return std::optional<int> (-1);
     }
diff --git a/gdb/testsuite/gdb.python/py-disasm.exp.tcl b/gdb/testsuite/gdb.python/py-disasm.exp.tcl
index 938326dd9cf..c5099bac22c 100644
--- a/gdb/testsuite/gdb.python/py-disasm.exp.tcl
+++ b/gdb/testsuite/gdb.python/py-disasm.exp.tcl
@@ -152,7 +152,10 @@ set test_plans \
 		   "Buffer returned from read_memory is sized $decimal instead of the expected $decimal"]] \
 	 [list "ResultOfWrongType" \
 	      [make_exception_pattern "TypeError" \
-		   "Result is not a DisassemblerResult."]] \
+		   "Result from Disassembler must be gdb.DisassemblerResult, not Blah."]] \
+	 [list "ResultOfVeryWrongType" \
+	      [make_exception_pattern "TypeError" \
+		   "Result from Disassembler must be gdb.DisassemblerResult, not Blah."]] \
 	 [list "ErrorCreatingTextPart_NoArgs" \
 	      [make_exception_pattern "TypeError" \
 		   [missing_arg_pattern "style" 1]]] \
diff --git a/gdb/testsuite/gdb.python/py-disasm.py b/gdb/testsuite/gdb.python/py-disasm.py
index 32d6aa7dc82..9761337550a 100644
--- a/gdb/testsuite/gdb.python/py-disasm.py
+++ b/gdb/testsuite/gdb.python/py-disasm.py
@@ -294,6 +294,24 @@ class ResultOfWrongType(TestDisassembler):
         return self.Blah(1, "ABC")
 
 
+class ResultOfVeryWrongType(TestDisassembler):
+    """Return something that is not a DisassemblerResult from disassemble
+    method.  The thing returned will raise an exception if used in an
+    isinstance() call, or in PyObject_IsInstance from C++.
+    """
+
+    class Blah:
+        def __init__(self):
+            pass
+
+        @property
+        def __class__(self):
+            raise RuntimeError("error from __class__ in Blah")
+
+    def disassemble(self, info):
+        return self.Blah()
+
+
 class TaggingDisassembler(TestDisassembler):
     """A simple disassembler that just tags the output."""