From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1879) id 491D2386183F; Thu, 14 Dec 2023 16:21:01 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 491D2386183F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1702570861; bh=32c5/SfvZb1abaBdcs7onIHkrfnEWtB3cuq/Atmn13U=; h=From:To:Subject:Date:From; b=HwuHbL8oiL7DV8fIf1ygY74oCqkGAW7nNuUuew+tDmEnUCFaevRuJXmYXg8sQ5eAi ZxfvCszaeqD7Uvw6pcovJ6UdrnM30OfE4nFSwz8GBkoMiYQR2JsSUZDX2/PQiRGjtv d5FcJwJVnVzFvGKO+tcvgfQ+tkQiQdvYQ4aDt3Eo= Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Simon Marchi To: gdb-cvs@sourceware.org Subject: [binutils-gdb] gdb: fix bugs in {get,put}_frame_register_bytes X-Act-Checkin: binutils-gdb X-Git-Author: Simon Marchi X-Git-Refname: refs/heads/master X-Git-Oldrev: 51e6b8cfd649013ae16a3d00f1451b2531ba6bc9 X-Git-Newrev: e94d1f726ff6271e826b598301cf3e759793ac1a Message-Id: <20231214162101.491D2386183F@sourceware.org> Date: Thu, 14 Dec 2023 16:21:01 +0000 (GMT) List-Id: https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3De94d1f726ff6= 271e826b598301cf3e759793ac1a commit e94d1f726ff6271e826b598301cf3e759793ac1a Author: Simon Marchi Date: Fri Dec 1 11:27:19 2023 -0500 gdb: fix bugs in {get,put}_frame_register_bytes =20 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`. =20 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. =20 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. =20 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. =20 [1] https://gitlab.com/gnutools/binutils-gdb/-/commit/bdec2917b1e94c719= 8ba39919f45060067952f43 =20 Change-Id: I43bd4a9f7fa8419d388a2b20bdc57d652688ddf8 Reviewed-By: John Baldwin Approved-By: Andrew Burgess Diff: --- gdb/frame.c | 63 ++++++++++++++++++++++-----------------------------------= ---- 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index 529453efa15..08ce2170543 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1483,9 +1483,6 @@ get_frame_register_bytes (frame_info_ptr frame, int r= egnum, int *optimizedp, int *unavailablep) { struct gdbarch *gdbarch =3D get_frame_arch (frame); - int i; - int maxsize; - int numregs; =20 /* Skip registers wholly inside of OFFSET. */ while (offset >=3D register_size (gdbarch, regnum)) @@ -1496,9 +1493,9 @@ get_frame_register_bytes (frame_info_ptr frame, int r= egnum, =20 /* 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 =3D -offset; - numregs =3D gdbarch_num_cooked_regs (gdbarch); - for (i =3D regnum; i < numregs; i++) + int maxsize =3D -offset; + int numregs =3D gdbarch_num_cooked_regs (gdbarch); + for (int i =3D regnum; i < numregs; i++) { int thissize =3D register_size (gdbarch, i); =20 @@ -1507,20 +1504,15 @@ get_frame_register_bytes (frame_info_ptr frame, int= regnum, maxsize +=3D thissize; } =20 - int len =3D 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 ()); =20 /* Copy the data. */ - while (len > 0) + while (!buffer.empty ()) { - int curr_len =3D register_size (gdbarch, regnum) - offset; - - if (curr_len > len) - curr_len =3D len; - - gdb_byte *myaddr =3D buffer.data (); + int curr_len =3D std::min (register_size (gdbarch, regnum) - of= fset, + buffer.size ()); =20 if (curr_len =3D=3D register_size (gdbarch, regnum)) { @@ -1528,8 +1520,8 @@ get_frame_register_bytes (frame_info_ptr frame, int r= egnum, CORE_ADDR addr; int realnum; =20 - 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; } =20 - 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); } =20 - myaddr +=3D curr_len; - len -=3D curr_len; + buffer =3D buffer.slice (curr_len); offset =3D 0; regnum++; } @@ -1579,36 +1570,28 @@ put_frame_register_bytes (frame_info_ptr frame, int= regnum, regnum++; } =20 - int len =3D buffer.size (); /* Copy the data. */ - while (len > 0) + while (!buffer.empty ()) { - int curr_len =3D register_size (gdbarch, regnum) - offset; + int curr_len =3D std::min (register_size (gdbarch, regnum) - of= fset, + buffer.size ()); =20 - if (curr_len > len) - curr_len =3D len; - - const gdb_byte *myaddr =3D buffer.data (); if (curr_len =3D=3D register_size (gdbarch, regnum)) - { - put_frame_register (frame, regnum, myaddr); - } + put_frame_register (frame, regnum, buffer.data ()); else { - struct value *value + value *value =3D frame_unwind_register_value (frame_info_ptr (frame->next), regnum); - gdb_assert (value !=3D NULL); + gdb_assert (value !=3D nullptr); =20 - 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); } =20 - myaddr +=3D curr_len; - len -=3D curr_len; + buffer =3D buffer.slice (curr_len); offset =3D 0; regnum++; }