From: Simon Marchi <simon.marchi@efficios.com>
To: gdb-patches@sourceware.org
Cc: Luis Machado <luis.machado@arm.com>,
John Baldwin <jhb@freebsd.org>,
Simon Marchi <simon.marchi@efficios.com>
Subject: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset
Date: Thu, 30 Nov 2023 16:20:46 -0500 [thread overview]
Message-ID: <20231130212057.722990-2-simon.marchi@efficios.com> (raw)
In-Reply-To: <20231130212057.722990-1-simon.marchi@efficios.com>
regcache::transfer_regset iterates over an array of regcache_map_entry,
transferring the registers (between regcache and buffer) described by
those entries. It stops either when it reaches the end of the
regcache_map_entry array (marked by a null entry) or (it seems like the
intent is) when it reaches the end of the buffer (in which case not all
described registers are transferred).
I said "seems like the intent is", because there appears to be a small
bug. transfer_regset is made of two loops:
foreach regcache_map_entry:
foreach register described by the regcache_map_entry:
if the register doesn't fit in the remainder of the buffer:
break
transfer register
When stopping because we have reached the end of the buffer, the break
only breaks out of the inner loop.
This problem causes some failures when I run tests such as
gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu). This is
partly due to aarch64_linux_iterate_over_regset_sections failing to add
a null terminator in its regcache_map_entry array, but I think there is
still a problem in transfer_regset.
The sequence to the crash is:
- The `regcache_map_entry za_regmap` object built in
aarch64_linux_iterate_over_regset_sections does not have a null
terminator.
- When the target does not have a ZA register,
aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
a size of 0 (it's actually pointless, but still it should work).
- transfer_regset gets called with a buffer size of 0.
- transfer_regset detects that the register to transfer wouldn't fit in
0 bytes, so it breaks out of the inner loop.
- The outer loop tries to go read the next regcache_map_entry, but
there isn't one, and we start reading garbage.
Obviously, this would get fixed by making
aarch64_linux_iterate_over_regset_sections use a null terminator (which
is what the following patch does). But I think that when detecting that
there is not enough buffer left for the current register,
transfer_regset should return, not only break out of the inner loop.
This is a kind of contrived scenario, but imagine we have these two
regcache_map_entry objects:
- 2 registers of 8 bytes
- 2 registers of 4 bytes
For some reason, the caller passes a buffer of 12 bytes.
transfer_regset will detect that the second 8 byte register does not
fit, and break out of the inner loop. However, it will then go try the
next regcache_map_entry. It will see that it can fit one 4 byte
register in the remaining buffer space, and transfer it from/to there.
This is very likely not an expected behavior, we wouldn't expect to
read/write this sequence of registers from/to the buffer.
In this example, whether passing a 12 bytes buffer makes sense or
whether it is a size computation bug in the caller, we don't know, but I
think that exiting as soon as a register doesn't fit is the sane thing
to do.
Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
---
gdb/regcache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 9dc354ec2b3a..e46a0b58f505 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
for (; count--; regno++, offs += slot_size)
{
if (offs + slot_size > size)
- break;
+ return;
transfer_regset_register (out_regcache, regno, in_buf, out_buf,
slot_size, offs);
--
2.43.0
next prev parent reply other threads:[~2023-11-30 21:21 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-30 21:20 [PATCH 0/2] Fix gdb.arch/aarch64-sme-core-*.exp failures Simon Marchi
2023-11-30 21:20 ` Simon Marchi [this message]
2023-11-30 23:42 ` [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset John Baldwin
2023-12-01 0:02 ` Simon Marchi
2023-12-01 9:13 ` Luis Machado
2023-11-30 21:20 ` [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c Simon Marchi
2023-11-30 23:44 ` John Baldwin
2023-12-01 0:03 ` Simon Marchi
2023-12-01 9:12 ` Luis Machado
2023-12-01 16:21 ` Simon Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231130212057.722990-2-simon.marchi@efficios.com \
--to=simon.marchi@efficios.com \
--cc=gdb-patches@sourceware.org \
--cc=jhb@freebsd.org \
--cc=luis.machado@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).