From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 311B43858D1E for ; Thu, 28 Apr 2022 01:45:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 311B43858D1E Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id DB9551E00D; Wed, 27 Apr 2022 21:45:42 -0400 (EDT) Message-ID: <80873420-8a71-111f-453c-f354381e0ad3@simark.ca> Date: Wed, 27 Apr 2022 21:45:42 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH] Fix crash in gdbpy_parse_register_id Content-Language: en-US To: Tom Tromey , gdb-patches@sourceware.org References: <20220427212742.4003557-1-tom@tromey.com> From: Simon Marchi In-Reply-To: <20220427212742.4003557-1-tom@tromey.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Apr 2022 01:45:44 -0000 > @@ -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_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