From patchwork Fri Dec 1 16:27:19 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 81138 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0224C385E449 for ; Fri, 1 Dec 2023 16:28:35 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id A1802385C322 for ; Fri, 1 Dec 2023 16:27:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A1802385C322 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=efficios.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=efficios.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org A1802385C322 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701448078; cv=none; b=OysgYCyDTJOSEaMN+X+amNZpFOZ9k/PPZI837oNYUGBSJaJiWtYuu0Cg4xzjTgwABWY7vIdY4ytrei/qm0zcsXxKyrAIewoMTwAPS1rx5inyaAOaHwtUA3Lw2J/aKX2c2Qbl22EWdmAHWuWX2BnSnVxuPy58wnoBTYVGzYgfn/o= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701448078; c=relaxed/simple; bh=wVPqzAmvuAtSNJir8Y7X9eB5HWLgPd0NVVOngu41xeI=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=xclH5gcNsndIky3JSPIebuMeUxEsV6oLZbs1QoFuAUvyHPTd+/U47VyipOUFn9BC3k5KPd7CwkngZVX5LDwf+5D2Gxj8Jwiv44MC+zCwaRbgAJNYSz0d+R0b5wA9YecUNS7LycgQ5W5dIYuMhW/LbDbBkSKG7NH+WeVYZ/N8KAc= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from smarchi-efficios.internal.efficios.com (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 103501E1A8; Fri, 1 Dec 2023 11:27:55 -0500 (EST) From: Simon Marchi To: gdb-patches@sourceware.org Cc: Luis Machado , John Baldwin , "Aktemur, Tankut Baris" , Simon Marchi , John Baldwin , Andrew Burgess Subject: [PATCH 06/24] gdb: fix bugs in {get,put}_frame_register_bytes Date: Fri, 1 Dec 2023 11:27:19 -0500 Message-ID: <20231201162751.741751-7-simon.marchi@efficios.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20231201162751.741751-1-simon.marchi@efficios.com> References: <20231201162751.741751-1-simon.marchi@efficios.com> MIME-Version: 1.0 X-Spam-Status: No, score=-3496.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_SOFTFAIL, 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.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org I found this only by inspection: the myaddr pointer in {get,put}_frame_register_bytes is reset to `buffer.data ()` at each iteration. This means that we will always use the bytes at the beginning of `buffer` to read or write to the registers, instead of progressing in `buffer`. Fix this by re-writing the functions to chip away the beginning of the buffer array_view as we progress in reading or writing the data. These bugs was introduced almost 3 years ago [1], and yet nobody complained. I'm wondering which architecture relies on that register "overflow" behavior (reading or writing multiple consecutive registers with one {get,put}_frame_register_bytes calls), and in which situation. I find these functions a bit dangerous, if a caller mis-calculates things, it could end up silently reading or writing to the next register, even if it's not the intent. If I could change it, I would prefer to have functions specifically made for that ({get,put}_frame_register_bytes_consecutive or something like that) and make {get,put}_frame_register_bytes only able to write within a single register (which I presume represents most of the use cases of the current {get,put}_frame_register_bytes). If a caller mis-calculates things and an overflow occurs while calling {get,put}_frame_register_bytes, it would hit an assert. The problem is knowing which callers rely on the overflow behavior and which don't. [1] https://gitlab.com/gnutools/binutils-gdb/-/commit/bdec2917b1e94c7198ba39919f45060067952f43 Change-Id: I43bd4a9f7fa8419d388a2b20bdc57d652688ddf8 Reviewed-By: John Baldwin Approved-By: Andrew Burgess --- gdb/frame.c | 63 +++++++++++++++++++---------------------------------- 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index 529453efa158..08ce21705432 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1483,9 +1483,6 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, int *optimizedp, int *unavailablep) { struct gdbarch *gdbarch = get_frame_arch (frame); - int i; - int maxsize; - int numregs; /* Skip registers wholly inside of OFFSET. */ while (offset >= register_size (gdbarch, regnum)) @@ -1496,9 +1493,9 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, /* Ensure that we will not read beyond the end of the register file. This can only ever happen if the debug information is bad. */ - maxsize = -offset; - numregs = gdbarch_num_cooked_regs (gdbarch); - for (i = regnum; i < numregs; i++) + int maxsize = -offset; + int numregs = gdbarch_num_cooked_regs (gdbarch); + for (int i = regnum; i < numregs; i++) { int thissize = register_size (gdbarch, i); @@ -1507,20 +1504,15 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, maxsize += thissize; } - int len = buffer.size (); - if (len > maxsize) + if (buffer.size () > maxsize) error (_("Bad debug information detected: " - "Attempt to read %d bytes from registers."), len); + "Attempt to read %zu bytes from registers."), buffer.size ()); /* Copy the data. */ - while (len > 0) + while (!buffer.empty ()) { - int curr_len = register_size (gdbarch, regnum) - offset; - - if (curr_len > len) - curr_len = len; - - gdb_byte *myaddr = buffer.data (); + int curr_len = std::min (register_size (gdbarch, regnum) - offset, + buffer.size ()); if (curr_len == register_size (gdbarch, regnum)) { @@ -1528,8 +1520,8 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, CORE_ADDR addr; int realnum; - frame_register (frame, regnum, optimizedp, unavailablep, - &lval, &addr, &realnum, myaddr); + frame_register (frame, regnum, optimizedp, unavailablep, &lval, + &addr, &realnum, buffer.data ()); if (*optimizedp || *unavailablep) return false; } @@ -1548,13 +1540,12 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, return false; } - memcpy (myaddr, value->contents_all ().data () + offset, - curr_len); + copy (value->contents_all ().slice (offset, curr_len), + buffer.slice (0, curr_len)); release_value (value); } - myaddr += curr_len; - len -= curr_len; + buffer = buffer.slice (curr_len); offset = 0; regnum++; } @@ -1579,36 +1570,28 @@ put_frame_register_bytes (frame_info_ptr frame, int regnum, regnum++; } - int len = buffer.size (); /* Copy the data. */ - while (len > 0) + while (!buffer.empty ()) { - int curr_len = register_size (gdbarch, regnum) - offset; + int curr_len = std::min (register_size (gdbarch, regnum) - offset, + buffer.size ()); - if (curr_len > len) - curr_len = len; - - const gdb_byte *myaddr = buffer.data (); if (curr_len == register_size (gdbarch, regnum)) - { - put_frame_register (frame, regnum, myaddr); - } + put_frame_register (frame, regnum, buffer.data ()); else { - struct value *value + value *value = frame_unwind_register_value (frame_info_ptr (frame->next), regnum); - gdb_assert (value != NULL); + gdb_assert (value != nullptr); - memcpy ((char *) value->contents_writeable ().data () + offset, - myaddr, curr_len); - put_frame_register (frame, regnum, - value->contents_raw ().data ()); + copy (buffer.slice (0, curr_len), + value->contents_writeable ().slice (offset, curr_len)); + put_frame_register (frame, regnum, value->contents_raw ().data ()); release_value (value); } - myaddr += curr_len; - len -= curr_len; + buffer = buffer.slice (curr_len); offset = 0; regnum++; }