From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 5F73D3858D1E for ; Wed, 8 Nov 2023 05:21:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5F73D3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 5F73D3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=132.207.4.11 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699420905; cv=none; b=DlQl8Sw9nY6T4l7OnMP9gPGk+LMiP32saHGmwJWO9OgQeT7BsZC7zO5+sUdq+fToin2c86+PGETQ6Ns0pcbvCPhIoYsAMVoW/3BodByutS+9A+3Bs1qnE0xgBTEzrvmH3Tj8BLUff7XDEjzeDRujYSE4ZGXrmMOI3DcpIcIkoCc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699420905; c=relaxed/simple; bh=8SX4P9U/z8qCKf4HMZhvtecIes2oJ4TOQcK58MgAFLM=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=T/MYxAipRhbLHVKz3haNiymgOmc7U2HENlABjUSFe5Fhh/o3WU6ZEGKf45qEEATdhKQwA2SqmAlNtTgHznj/koLO+ci8MXsXPVjHoSPd/IlKqWrBYCO4fSejXzyki0ckwWlLtktMx/6OieBTEuDCQE9kzCsmBXCedIi3H5O9Z94= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 3A85LcHJ016954 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 8 Nov 2023 00:21:42 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 3A85LcHJ016954 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1699420902; bh=kG2kgDX0uZdraS1+5yRYFvl3G6YP/j4NmjmZrY3oKWY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=H7KUiW36Jln/q2dcI1HImcLg8pEQV8sxhwbo+zF6/3q/DT5YNEver7vbH1cuBNMes zSS2Rc1gs2bw8r0U2oxKS3lUp82sdBESye0bT0Y7e6lZbI9B3d8BZQtbPsPcxRXBtE yRK5F6x0PO7WGZGc0yxhXAY/OFl8l+HXc/Kx3RV0= Received: from simark.localdomain (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (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 2F7301E1A8; Wed, 8 Nov 2023 00:12:28 -0500 (EST) From: Simon Marchi To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 06/24] gdb: fix bugs in {get,put}_frame_register_bytes Date: Wed, 8 Nov 2023 00:00:50 -0500 Message-ID: <20231108051222.1275306-7-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.42.1 In-Reply-To: <20231108051222.1275306-1-simon.marchi@polymtl.ca> References: <20231108051222.1275306-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 8 Nov 2023 05:21:38 +0000 X-Spam-Status: No, score=-3188.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,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 List-Id: From: Simon Marchi 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 --- gdb/frame.c | 63 +++++++++++++++++++---------------------------------- 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index afadb8ac4e73..b3d99163b4dc 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1482,9 +1482,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)) @@ -1495,9 +1492,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); @@ -1506,20 +1503,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)) { @@ -1527,8 +1519,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; } @@ -1547,13 +1539,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++; } @@ -1578,36 +1569,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++; } -- 2.42.1