From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gproxy1-pub.mail.unifiedlayer.com (gproxy1-pub.mail.unifiedlayer.com [69.89.25.95]) by sourceware.org (Postfix) with ESMTPS id C445E385E036 for ; Sat, 11 Jun 2022 17:38:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C445E385E036 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com Received: from cmgw11.mail.unifiedlayer.com (unknown [10.0.90.126]) by progateway3.mail.pro1.eigbox.com (Postfix) with ESMTP id 1129F10048167 for ; Sat, 11 Jun 2022 17:38:25 +0000 (UTC) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTP id 0544obUevrOQ90544oi0xn; Sat, 11 Jun 2022 17:38:25 +0000 X-Authority-Reason: nr=8 X-Authority-Analysis: v=2.4 cv=CpF6zl0D c=1 sm=1 tr=0 ts=62a4d311 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=dLZJa+xiwSxG16/P+YVxDGlgEgI=:19 a=JPEYwPQDsx4A:10:nop_rcvd_month_year a=Qbun_eYptAEA:10:endurance_base64_authed_username_1 a=zstS-IiYAAAA:8 a=bFCFwW1K911IKleOHlEA:9 a=4G6NA9xxw8l3yy4pmD5M:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date:References :Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=JUge0w+PYyyLk0UsG0ICESHtuAsamP2aAbbWQ/EBaYM=; b=dXRnHJSSGq6jRkNPnhVQ2JRyiH 5SFtca8AA20CFWLyaq6GjawrgHlSN7iIbiwuvec2/ExMV1FpS4+vwsHcoodIR23YMuitYzDcO4aM/ IOGh9WeaxCPgnk8N6VJuerBFe; Received: from 71-211-171-143.hlrn.qwest.net ([71.211.171.143]:35080 helo=prentzel) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1o0544-001CYr-Bv; Sat, 11 Jun 2022 11:38:24 -0600 From: Tom Tromey To: Simon Marchi Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [PATCH] Fix crash in gdbpy_parse_register_id References: <20220427212742.4003557-1-tom@tromey.com> <80873420-8a71-111f-453c-f354381e0ad3@simark.ca> X-Attribution: Tom Date: Sat, 11 Jun 2022 11:38:23 -0600 In-Reply-To: <80873420-8a71-111f-453c-f354381e0ad3@simark.ca> (Simon Marchi's message of "Wed, 27 Apr 2022 21:45:42 -0400") Message-ID: <87czff14og.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 71.211.171.143 X-Source-L: No X-Exim-ID: 1o0544-001CYr-Bv X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 71-211-171-143.hlrn.qwest.net (prentzel) [71.211.171.143]:35080 X-Source-Auth: tom+tromey.com X-Email-Count: 2 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-Spam-Status: No, score=-3029.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, JMQ_SPF_NEUTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Sat, 11 Jun 2022 17:38:27 -0000 >> + 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 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_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