From bdc8cfc1e43ebc4029cf130c678b9e1a4e4e5682 Mon Sep 17 00:00:00 2001 From: Tom Tromey Date: Wed, 27 Apr 2022 15:22:56 -0600 Subject: [PATCH] Fix crash in gdbpy_parse_register_id I noticed that gdbpy_parse_register_id would assert if passed a Python object of a type it was not expecting. The included test case shows this crash. This patch fixes the problem and also changes gdbpy_parse_register_id to be more "Python-like" -- it always ensures the Python error is set when it fails, and the callers now simply propagate the existing exception. --- gdb/python/py-frame.c | 5 +---- gdb/python/py-registers.c | 22 +++++++++++++++------- gdb/python/py-unwind.c | 10 ++-------- gdb/python/python-internal.h | 3 ++- gdb/testsuite/gdb.python/py-frame.exp | 6 ++++++ gdb/testsuite/gdb.python/py-unwind.exp | 6 ++++++ gdb/testsuite/gdb.python/py-unwind.py | 16 ++++++++++++++++ 7 files changed, 48 insertions(+), 20 deletions(-) diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c index 9a28c36c1cc..8bd2e0bc6da 100644 --- a/gdb/python/py-frame.c +++ b/gdb/python/py-frame.c @@ -253,10 +253,7 @@ frapy_read_register (PyObject *self, PyObject *args) if (!gdbpy_parse_register_id (get_frame_arch (frame), pyo_reg_id, ®num)) - { - PyErr_SetString (PyExc_ValueError, "Bad register"); - return NULL; - } + return nullptr; gdb_assert (regnum >= 0); val = value_of_register (regnum, frame); diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c index f22575a28c2..fe7481cea9e 100644 --- a/gdb/python/py-registers.c +++ b/gdb/python/py-registers.c @@ -381,21 +381,27 @@ gdbpy_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id, { *reg_num = user_reg_map_name_to_regnum (gdbarch, reg_name.get (), strlen (reg_name.get ())); - return *reg_num >= 0; + if (*reg_num >= 0) + return true; + PyErr_SetString (PyExc_ValueError, "Bad register"); } } /* The register could be its internal GDB register number. */ else if (PyLong_Check (pyo_reg_id)) { long value; - if (gdb_py_int_as_long (pyo_reg_id, &value) && (int) value == value) + if (gdb_py_int_as_long (pyo_reg_id, &value) == 0) { - if (user_reg_map_regnum_to_name (gdbarch, value) != NULL) - { - *reg_num = (int) value; - return true; - } + /* Nothing -- error. */ } + else if ((int) value == value + && user_reg_map_regnum_to_name (gdbarch, value) != NULL) + { + *reg_num = (int) value; + return true; + } + else + PyErr_SetString (PyExc_ValueError, "Bad register"); } /* The register could be a gdb.RegisterDescriptor object. */ else if (PyObject_IsInstance (pyo_reg_id, @@ -412,6 +418,8 @@ gdbpy_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id, PyErr_SetString (PyExc_ValueError, _("Invalid Architecture in RegisterDescriptor")); } + else + PyErr_SetString (PyExc_TypeError, _("Invalid type for register")); gdb_assert (PyErr_Occurred ()); return false; diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c index fb9466102c4..dcb1d7abbcf 100644 --- a/gdb/python/py-unwind.c +++ b/gdb/python/py-unwind.c @@ -260,10 +260,7 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args) &pyo_reg_id, &pyo_reg_value)) return NULL; if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, ®num)) - { - PyErr_SetString (PyExc_ValueError, "Bad register"); - return NULL; - } + return nullptr; /* If REGNUM identifies a user register then *maybe* we can convert this to a real (i.e. non-user) register. The maybe qualifier is because we @@ -381,10 +378,7 @@ pending_framepy_read_register (PyObject *self, PyObject *args) if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id)) return NULL; if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, ®num)) - { - PyErr_SetString (PyExc_ValueError, "Bad register"); - return NULL; - } + return nullptr; try { diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h index 800b03a105c..d624b23fdc5 100644 --- a/gdb/python/python-internal.h +++ b/gdb/python/python-internal.h @@ -819,7 +819,8 @@ typedef std::unique_ptr Py_buffer_up; If a register is parsed successfully then *REG_NUM will have been updated, and true is returned. Otherwise the contents of *REG_NUM are - undefined, and false is returned. + undefined, and false is returned. When false is returned, the + Python error is set. The PYO_REG_ID object can be a string, the name of the register. This is the slowest approach as GDB has to map the name to a number for each diff --git a/gdb/testsuite/gdb.python/py-frame.exp b/gdb/testsuite/gdb.python/py-frame.exp index 4991e8a0c5d..56e1ecdcedd 100644 --- a/gdb/testsuite/gdb.python/py-frame.exp +++ b/gdb/testsuite/gdb.python/py-frame.exp @@ -134,3 +134,9 @@ gdb_test "python print(gdb.selected_frame().language())" "c" gdb_test "set language ada" gdb_test "python print(gdb.selected_frame().language())" "c" \ "frame language is not affected by global language" + +# This previously caused a crash -- the implementation was missing the +# case where a register had an unexpected type. +gdb_test "python print(gdb.selected_frame().read_register(list()))" \ + ".*Invalid type for register.*" \ + "test Frame.read_register with list" diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp index cdf9034b1bd..798e76525db 100644 --- a/gdb/testsuite/gdb.python/py-unwind.exp +++ b/gdb/testsuite/gdb.python/py-unwind.exp @@ -57,3 +57,9 @@ gdb_test_sequence "where" "Backtrace restored by unwinder" { # Check that the Python unwinder frames can be flushed / released. gdb_test "maint flush register-cache" "Register cache flushed\\." "flush frames" + +# Check that invalid register names cause errors. +gdb_test "python print(add_saved_register_error)" "True" \ + "add_saved_register error" +gdb_test "python print(read_register_error)" "True" \ + "read_register error" diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py index 15dba59f624..319bb630c1c 100644 --- a/gdb/testsuite/gdb.python/py-unwind.py +++ b/gdb/testsuite/gdb.python/py-unwind.py @@ -17,6 +17,11 @@ import gdb from gdb.unwinder import Unwinder +# These are set to test whether invalid register names cause an error. +add_saved_register_error = False +read_register_error = False + + class FrameId(object): def __init__(self, sp, pc): self._sp = sp @@ -101,6 +106,12 @@ class TestUnwinder(Unwinder): previous_ip = self._read_word(bp + 8) previous_sp = bp + 16 + try: + pending_frame.read_register("nosuchregister") + except ValueError: + global read_register_error + read_register_error = True + frame_id = FrameId( pending_frame.read_register(TestUnwinder.AMD64_RSP), pending_frame.read_register(TestUnwinder.AMD64_RIP), @@ -109,6 +120,11 @@ class TestUnwinder(Unwinder): unwind_info.add_saved_register(TestUnwinder.AMD64_RBP, previous_bp) unwind_info.add_saved_register("rip", previous_ip) unwind_info.add_saved_register("rsp", previous_sp) + try: + unwind_info.add_saved_register("nosuchregister", previous_sp) + except ValueError: + global add_saved_register_error + add_saved_register_error = True return unwind_info except (gdb.error, RuntimeError): return None -- 2.30.2