* [PATCH] Fix crash in gdbpy_parse_register_id @ 2022-04-27 21:27 Tom Tromey 2022-04-28 1:45 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Tom Tromey @ 2022-04-27 21:27 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey 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 | 5 +++++ 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c index d07158a5ec6..933bdc773eb 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 NULL; 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 bbb322f068c..c7ea529bdde 100644 --- a/gdb/python/py-registers.c +++ b/gdb/python/py-registers.c @@ -386,21 +386,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, @@ -417,6 +423,8 @@ gdbpy_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id, PyErr_SetString (PyExc_ValueError, _("Invalid Architecture in RegisterDescriptor")); } + else + PyErr_SetString (PyExc_ValueError, _("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 b2fd1402e93..e2cd67a0785 100644 --- a/gdb/python/py-unwind.c +++ b/gdb/python/py-unwind.c @@ -262,10 +262,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 NULL; /* 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 @@ -383,10 +380,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; - } + PyErr_SetString (PyExc_ValueError, "Bad register"); try { diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h index d947b96033b..dffcd3f1b7f 100644 --- a/gdb/python/python-internal.h +++ b/gdb/python/python-internal.h @@ -799,7 +799,8 @@ typedef std::unique_ptr<Py_buffer, Py_buffer_deleter> 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 b91ffe62a83..881219342e3 100644 --- a/gdb/testsuite/gdb.python/py-frame.exp +++ b/gdb/testsuite/gdb.python/py-frame.exp @@ -128,3 +128,8 @@ if { $pc != "" } { " = True" \ "test Frame.read_register($pc)" } + +# This previously caused a crash. +gdb_test "python print(gdb.selected_frame().read_register(list()))" \ + ".*Invalid type for register.*" \ + "test Frame.read_register with list" -- 2.34.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix crash in gdbpy_parse_register_id 2022-04-27 21:27 [PATCH] Fix crash in gdbpy_parse_register_id Tom Tromey @ 2022-04-28 1:45 ` Simon Marchi 2022-04-28 2:11 ` Tom Tromey 2022-06-11 17:38 ` Tom Tromey 0 siblings, 2 replies; 5+ messages in thread From: Simon Marchi @ 2022-04-28 1:45 UTC (permalink / raw) To: Tom Tromey, gdb-patches > @@ -417,6 +423,8 @@ gdbpy_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id, > PyErr_SetString (PyExc_ValueError, > _("Invalid Architecture in RegisterDescriptor")); > } > + else > + PyErr_SetString (PyExc_ValueError, _("Invalid type for register")); I think this one should be a TypeError. We could argue that it is an API break to change the exception type thrown when passing an argument with the wrong type, but given that it previously crashed GDB, I don't think anybody is relying on it. > diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c > index b2fd1402e93..e2cd67a0785 100644 > --- a/gdb/python/py-unwind.c > +++ b/gdb/python/py-unwind.c > @@ -262,10 +262,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 NULL; > > /* 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 > @@ -383,10 +380,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; > - } > + PyErr_SetString (PyExc_ValueError, "Bad register"); It seems like you kept the wrong line here? If this is really an error and no test has caught this, it means we're lacking some coverage. > > try > { > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h > index d947b96033b..dffcd3f1b7f 100644 > --- a/gdb/python/python-internal.h > +++ b/gdb/python/python-internal.h > @@ -799,7 +799,8 @@ typedef std::unique_ptr<Py_buffer, Py_buffer_deleter> 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 b91ffe62a83..881219342e3 100644 > --- a/gdb/testsuite/gdb.python/py-frame.exp > +++ b/gdb/testsuite/gdb.python/py-frame.exp > @@ -128,3 +128,8 @@ if { $pc != "" } { > " = True" \ > "test Frame.read_register($pc)" > } > + > +# This previously caused a crash. This comment is a bit too broad, can you make it a bit more precise? What is it we are testing exactly? I suppose "passing an object with an unexpected type to read_register". Simon ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix crash in gdbpy_parse_register_id 2022-04-28 1:45 ` Simon Marchi @ 2022-04-28 2:11 ` Tom Tromey 2022-06-11 17:38 ` Tom Tromey 1 sibling, 0 replies; 5+ messages in thread From: Tom Tromey @ 2022-04-28 2:11 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, gdb-patches >> + PyErr_SetString (PyExc_ValueError, "Bad register"); Simon> It seems like you kept the wrong line here? If this is really an error Simon> and no test has caught this, it means we're lacking some coverage. Geez. I will look into that. I used to periodically do coverage analysis of gdb but lately lcov doesn't seem to work for me and I haven't bothered to track it down. Tom ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix crash in gdbpy_parse_register_id 2022-04-28 1:45 ` Simon Marchi 2022-04-28 2:11 ` Tom Tromey @ 2022-06-11 17:38 ` Tom Tromey 2022-08-21 14:01 ` Tom Tromey 1 sibling, 1 reply; 5+ messages in thread From: Tom Tromey @ 2022-06-11 17:38 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, gdb-patches >> + PyErr_SetString (PyExc_ValueError, _("Invalid type for register")); Simon> I think this one should be a TypeError. Fixed. >> + PyErr_SetString (PyExc_ValueError, "Bad register"); Simon> It seems like you kept the wrong line here? If this is really an error Simon> and no test has caught this, it means we're lacking some coverage. Yeah, oops. I added a test. >> +# This previously caused a crash. Simon> This comment is a bit too broad, can you make it a bit more precise? Simon> What is it we are testing exactly? I suppose "passing an object with an Simon> unexpected type to read_register". I did this. New patch appended. Tom commit 310a3620ac6f86bbc0bc2c400a3df4202adcfe41 Author: Tom Tromey <tom@tromey.com> Date: Wed Apr 27 15:22:56 2022 -0600 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. 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 bbb322f068c..bac85c1ecff 100644 --- a/gdb/python/py-registers.c +++ b/gdb/python/py-registers.c @@ -386,21 +386,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, @@ -417,6 +423,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 b2fd1402e93..15b22ab9b30 100644 --- a/gdb/python/py-unwind.c +++ b/gdb/python/py-unwind.c @@ -262,10 +262,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 @@ -383,10 +380,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 217bc15bb28..2136c72faae 100644 --- a/gdb/python/python-internal.h +++ b/gdb/python/python-internal.h @@ -799,7 +799,8 @@ typedef std::unique_ptr<Py_buffer, Py_buffer_deleter> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix crash in gdbpy_parse_register_id 2022-06-11 17:38 ` Tom Tromey @ 2022-08-21 14:01 ` Tom Tromey 0 siblings, 0 replies; 5+ messages in thread From: Tom Tromey @ 2022-08-21 14:01 UTC (permalink / raw) To: Tom Tromey; +Cc: Simon Marchi, gdb-patches [...] Tom> I did this. Tom> New patch appended. Tom> Fix crash in gdbpy_parse_register_id Tom> I noticed that gdbpy_parse_register_id would assert if passed a Python Tom> object of a type it was not expecting. The included test case shows Tom> this crash. This patch fixes the problem and also changes Tom> gdbpy_parse_register_id to be more "Python-like" -- it always ensures Tom> the Python error is set when it fails, and the callers now simply Tom> propagate the existing exception. I'm checking this in now. Tom ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-21 14:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-27 21:27 [PATCH] Fix crash in gdbpy_parse_register_id Tom Tromey 2022-04-28 1:45 ` Simon Marchi 2022-04-28 2:11 ` Tom Tromey 2022-06-11 17:38 ` Tom Tromey 2022-08-21 14:01 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).