[patch] [python] PR python/15461 (gate architecture calls)

[patch] [python] PR python/15461 (gate architecture calls)

[Phil Muldoon]

This patch gates calls to architecture.name and
architecture.disassemble.  If the underlying GDB architecture is NULL,
calls to those Python functions will result in an assert call.

Something I thought about is not allowing gdb.Architecture to be
instantiated directly, but I could not think of a clean way to do
this.  Also, all other GDB Python classes can be instantiated
(though, not very usefully, IE foo = gdb.Frame())

In any case, this patch gates those calls mentioned above.

Cheers,

Phil

2013-08-28  Phil Muldoon  <pmuldoon@redhat.com>

	PR python/15461

	* python/py-arch.c (ARCHPY_REQUIRE_VALID): New macro.
	(archpy_name): Check for valid architecture.
	(archpy_disassemble): Ditto.


2013-08-28  Phil Muldoon  <pmuldoon@redhat.com>

	* gdb.python/py-arch.exp: Tests for invalid architecture.

--

diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
index 7098a8a..2775c9d 100644
--- a/gdb/python/py-arch.c
+++ b/gdb/python/py-arch.c
@@ -30,6 +30,14 @@ typedef struct arch_object_type_object {
 
 static struct gdbarch_data *arch_object_data = NULL;
 
+/* Require a valid Architecture.  */
+#define ARCHPY_REQUIRE_VALID (arch_obj, arch)		\
+  do {							\
+    arch = arch_object_to_gdbarch (arch_obj);		\
+    if (arch == NULL)					\
+      error (_("Architecture is invalid."));		\
+  } while (0)
+
 static PyTypeObject arch_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("arch_object");
 
@@ -82,9 +90,19 @@ gdbarch_to_arch_object (struct gdbarch *gdbarch)
 static PyObject *
 archpy_name (PyObject *self, PyObject *args)
 {
-  struct gdbarch *gdbarch = arch_object_to_gdbarch (self);
-  const char *name = (gdbarch_bfd_arch_info (gdbarch))->printable_name;
-  PyObject *py_name = PyString_FromString (name);
+  struct gdbarch *gdbarch = NULL;
+  const char *name;
+  PyObject *py_name;
+  volatile struct gdb_exception except;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      ARCHPY_REQUIRE_VALID (self, gdbarch);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
+
+  name = (gdbarch_bfd_arch_info (gdbarch))->printable_name;
+  py_name = PyString_FromString (name);
 
   return py_name;
 }
@@ -104,7 +122,14 @@ archpy_disassemble (PyObject *self, PyObject *args, PyObject *kw)
   gdb_py_ulongest start_temp;
   long count = 0, i;
   PyObject *result_list, *end_obj = NULL, *count_obj = NULL;
-  struct gdbarch *gdbarch = arch_object_to_gdbarch (self);
+  struct gdbarch *gdbarch = NULL;
+  volatile struct gdb_exception except;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      ARCHPY_REQUIRE_VALID (self, gdbarch);
+    }
+  GDB_PY_HANDLE_EXCEPTION (except);
 
   if (!PyArg_ParseTupleAndKeywords (args, kw, GDB_PY_LLU_ARG "|OO", keywords,
                                     &start_temp, &end_obj, &count_obj))
diff --git a/gdb/testsuite/gdb.python/py-arch.exp b/gdb/testsuite/gdb.python/py-arch.exp
index 4e736b8..0d32ef7 100644
--- a/gdb/testsuite/gdb.python/py-arch.exp
+++ b/gdb/testsuite/gdb.python/py-arch.exp
@@ -12,6 +12,7 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib gdb-python.exp
 
 standard_testfile
 
@@ -26,6 +27,14 @@ if ![runto_main] {
    return -1
 }
 
+# Test python/15461.  Invalid architectures should not trigger an
+# internal GDB assert.
+gdb_py_test_silent_cmd "python empty = gdb.Architecture()" "get empty arch" 0
+gdb_test "python print(empty.name())" ".*Architecture is invalid.*" \
+    "Test empty architecture.name does not trigger an assert"
+gdb_test "python print(empty.disassemble())" ".*Architecture is invalid.*" \
+    "Test empty architecture.disassemble does not trigger an assert"
+
 gdb_py_test_silent_cmd "python frame = gdb.selected_frame()" "get frame" 0
 gdb_py_test_silent_cmd "python arch = frame.architecture()" "get arch" 0
 gdb_py_test_silent_cmd "python pc = frame.pc()" "get pc" 0

	

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Tom Tromey]

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

Phil> This patch gates calls to architecture.name and
Phil> architecture.disassemble.  If the underlying GDB architecture is NULL,
Phil> calls to those Python functions will result in an assert call.

Thanks.

Phil> +/* Require a valid Architecture.  */
Phil> +#define ARCHPY_REQUIRE_VALID (arch_obj, arch)		\
Phil> +  do {							\
Phil> +    arch = arch_object_to_gdbarch (arch_obj);		\
Phil> +    if (arch == NULL)					\
Phil> +      error (_("Architecture is invalid."));		\
Phil> +  } while (0)

I think it would be simpler to either:

* Have this set the python exception and not call error, or
* Likewise but just have it be done in arch_object_to_gdbarch.

With the current approach you have to have a try/catch and then throw an
exception; but really all you want is to set the python exception --
which is cheaper to do directly.

Phil>  # You should have received a copy of the GNU General Public License
Phil>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
Phil> +load_lib gdb-python.exp

It would be handy to get this hunk in sooner.
It's obvious fwiw.

Tom

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Phil Muldoon]

On 28/08/13 15:52, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> 
> Phil> This patch gates calls to architecture.name and
> Phil> architecture.disassemble.  If the underlying GDB architecture is NULL,
> Phil> calls to those Python functions will result in an assert call.
> 
> Thanks.
> 
> Phil> +/* Require a valid Architecture.  */
> Phil> +#define ARCHPY_REQUIRE_VALID (arch_obj, arch)		\
> Phil> +  do {							\
> Phil> +    arch = arch_object_to_gdbarch (arch_obj);		\
> Phil> +    if (arch == NULL)					\
> Phil> +      error (_("Architecture is invalid."));		\
> Phil> +  } while (0)
> 
> I think it would be simpler to either:
> 
> * Have this set the python exception and not call error, or
> * Likewise but just have it be done in arch_object_to_gdbarch.
> 
> With the current approach you have to have a try/catch and then throw an
> exception; but really all you want is to set the python exception --
> which is cheaper to do directly.

I don't disagree on the efficiency argument, but my goal here was to
follow the pattern that other objects use to determine validity of the
underlying GDB data. To bring a sense of uniformity to how we do
things in Python. So how we check a gdb.Frame's, et al, validity, the
pattern will be the same, as far as possible, for other objects.  In
that sense, to me at least, the uniformity of how we do this trumps
expectancy of efficiency. I don't really want to argue the point too
much, as I really don't have very strong feelings either way.

Additionally, I could only see this routine happening in the test-case
derived from the bug, so I think this code would rarely run.

Cheers,

Phil

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Tom Tromey]

Phil> I don't disagree on the efficiency argument, but my goal here was to
Phil> follow the pattern that other objects use to determine validity of the
Phil> underlying GDB data. To bring a sense of uniformity to how we do
Phil> things in Python. So how we check a gdb.Frame's, et al, validity, the
Phil> pattern will be the same, as far as possible, for other objects.

Ok.

If you look at other ones, they set the Python exception.  At least that
is true for py-block.c (twice), py-inferior.c, py-inthread.c,
py-symbol.c, py-symtab.c (twice), etc.

FWIW I don't mind inconsistency in these little details.  What matters
is the context in which the macro is most useful.

Tom

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Tom Tromey]

Phil> Something I thought about is not allowing gdb.Architecture to be
Phil> instantiated directly, but I could not think of a clean way to do
Phil> this.

Did you think of any way?
It seems like a good thing to prevent.

Tom

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Paul_Koning]

On Aug 28, 2013, at 1:54 PM, Tom Tromey <tromey@redhat.com> wrote:

> Phil> Something I thought about is not allowing gdb.Architecture to be
> Phil> instantiated directly, but I could not think of a clean way to do
> Phil> this.
> 
> Did you think of any way?
> It seems like a good thing to prevent.

Ditto for all the existing types for which direct instantiation doesn't do anything useful.

I haven't tried this, but judging from the Python docs, if you point the tp_new field of the type object to a function that sets a suitable Python exception, that should do the job.

	paul

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Phil Muldoon]

On 28/08/13 19:13, Paul_Koning@Dell.com wrote:
> 
> On Aug 28, 2013, at 1:54 PM, Tom Tromey <tromey@redhat.com> wrote:
> 
>> Phil> Something I thought about is not allowing gdb.Architecture to be
>> Phil> instantiated directly, but I could not think of a clean way to do
>> Phil> this.
>>
>> Did you think of any way?
>> It seems like a good thing to prevent.
> 
> Ditto for all the existing types for which direct instantiation doesn't do anything useful.
> 
> I haven't tried this, but judging from the Python docs, if you point the tp_new field of the type object to a function that sets a suitable Python exception, that should do the job.

I was not able to make the tp_new call figure out where it was being
instantiated from.

So I could not tell the difference between:

foo = gdb.selected_frame()

and

foo = gdb.Frame()

There is a way if the class constructor takes arguments (ie, foo =
gdb.Value(2), but that case is valid in any case).

But I only spent a very brief time on it.  I am sure there is a way to
do it.  I will look at it in a little more detail now we think we
should make this happen.

Cheers

Ohil

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Phil Muldoon]

On 28/08/13 16:54, Tom Tromey wrote:
> Phil> I don't disagree on the efficiency argument, but my goal here was to
> Phil> follow the pattern that other objects use to determine validity of the
> Phil> underlying GDB data. To bring a sense of uniformity to how we do
> Phil> things in Python. So how we check a gdb.Frame's, et al, validity, the
> Phil> pattern will be the same, as far as possible, for other objects.
> 
> Ok.
> 
> If you look at other ones, they set the Python exception.  At least that
> is true for py-block.c (twice), py-inferior.c, py-inthread.c,
> py-symbol.c, py-symtab.c (twice), etc.
> 
> FWIW I don't mind inconsistency in these little details.  What matters
> is the context in which the macro is most useful.

For some reason I totally misread your original comment.  I re-read
the thread this morning, and found my mistake.  Mea Cupla.  I thought
you did not want the macro, but that was not the case.  Your objection
was to the error() call, and was totally correct.  Not sure why I read
that email so incorrectly.  Apologies.

So just ignore my previous comments ;)

Updated the patch.  ChangeLog remains the same.

Cheers,

Phil

--

diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
index 7098a8a..a31ffdd 100644
--- a/gdb/python/py-arch.c
+++ b/gdb/python/py-arch.c
@@ -30,6 +30,18 @@ typedef struct arch_object_type_object {
 
 static struct gdbarch_data *arch_object_data = NULL;
 
+/* Require a valid Architecture.  */
+#define ARCHPY_REQUIRE_VALID(arch_obj, arch)			\
+  do {								\
+    arch = arch_object_to_gdbarch (arch_obj);			\
+    if (arch == NULL)						\
+      {							\
+	PyErr_SetString (PyExc_RuntimeError,			\
+			 _("Architecture is invalid."));	\
+	return NULL;						\
+      }							\
+  } while (0)
+
 static PyTypeObject arch_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("arch_object");
 
@@ -82,9 +94,14 @@ gdbarch_to_arch_object (struct gdbarch *gdbarch)
 static PyObject *
 archpy_name (PyObject *self, PyObject *args)
 {
-  struct gdbarch *gdbarch = arch_object_to_gdbarch (self);
-  const char *name = (gdbarch_bfd_arch_info (gdbarch))->printable_name;
-  PyObject *py_name = PyString_FromString (name);
+  struct gdbarch *gdbarch = NULL;
+  const char *name;
+  PyObject *py_name;
+
+  ARCHPY_REQUIRE_VALID (self, gdbarch);
+
+  name = (gdbarch_bfd_arch_info (gdbarch))->printable_name;
+  py_name = PyString_FromString (name);
 
   return py_name;
 }
@@ -104,7 +121,9 @@ archpy_disassemble (PyObject *self, PyObject *args, PyObject *kw)
   gdb_py_ulongest start_temp;
   long count = 0, i;
   PyObject *result_list, *end_obj = NULL, *count_obj = NULL;
-  struct gdbarch *gdbarch = arch_object_to_gdbarch (self);
+  struct gdbarch *gdbarch = NULL;
+
+  ARCHPY_REQUIRE_VALID (self, gdbarch);
 
   if (!PyArg_ParseTupleAndKeywords (args, kw, GDB_PY_LLU_ARG "|OO", keywords,
                                     &start_temp, &end_obj, &count_obj))
diff --git a/gdb/testsuite/gdb.python/py-arch.exp b/gdb/testsuite/gdb.python/py-arch.exp
index 4e736b8..6fff256 100644
--- a/gdb/testsuite/gdb.python/py-arch.exp
+++ b/gdb/testsuite/gdb.python/py-arch.exp
@@ -26,6 +26,14 @@ if ![runto_main] {
    return -1
 }
 
+# Test python/15461.  Invalid architectures should not trigger an
+# internal GDB assert.
+gdb_py_test_silent_cmd "python empty = gdb.Architecture()" "get empty arch" 0
+gdb_test "python print(empty.name())" ".*Architecture is invalid.*" \
+    "Test empty architecture.name does not trigger an assert"
+gdb_test "python print(empty.disassemble())" ".*Architecture is invalid.*" \
+    "Test empty architecture.disassemble does not trigger an assert"
+
 gdb_py_test_silent_cmd "python frame = gdb.selected_frame()" "get frame" 0
 gdb_py_test_silent_cmd "python arch = frame.architecture()" "get arch" 0
 gdb_py_test_silent_cmd "python pc = frame.pc()" "get pc" 0

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Tom Tromey]

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

Phil> Updated the patch.  ChangeLog remains the same.

Thanks, Phil.
This is ok.

Tom

Re: [patch] [python] PR python/15461 (gate architecture calls)

[Phil Muldoon]

On 29/08/13 15:58, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> 
> Phil> Updated the patch.  ChangeLog remains the same.
> 
> Thanks, Phil.
> This is ok.
> 
> Tom
> 


So committed, thanks.

Cheers,

Phil