From: Simon Marchi Date: Fri, 1 Dec 2023 16:27:19 +0000 (-0500) Subject: gdb: fix bugs in {get,put}_frame_register_bytes X-Git-Tag: binutils-2_42~596 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e94d1f726ff6271e826b598301cf3e759793ac1a;p=thirdparty%2Fbinutils-gdb.git gdb: fix bugs in {get,put}_frame_register_bytes 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 --- 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 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++; }