From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 2CC633858D38 for ; Fri, 1 Dec 2023 00:02:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2CC633858D38 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 2CC633858D38 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=1701388952; cv=none; b=BgoVttYRe5johepWvCmr2TfDCC05rVkAXK5JynawUWbwLhkCDeE1dAsotrXVgG8sDJmyBUA0S08jX8dsQLcTJWLz/tc/vrzPCdtwCChIONiNl31vrwIY5/mT1JDQ3FgUnR0IOnc49FOD7uj9ALc+znI7UPw+Uz1pi4bhW2Mbr94= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701388952; c=relaxed/simple; bh=mo2FEI1Ch2XulBCc86lUV6EfQOdmKuj1cbnDQlcOp/s=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=jawL5JrLP5+20ASEM4TXjdWmxCan0xRzNq+tI7PHrIOlG0NQ84u7EGUYNz6PB0k1nVHzaTcjozjLJc0KvTWw2cQB+3Ee5Pm7Y+VIpu/MrOo+LNb60Bm1ORh5Q1I2K5VJhLQO+jQ6LoDxl6YS49sjYCfqxVEMJ9oAwUQdpviUcn4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1701388950; bh=mo2FEI1Ch2XulBCc86lUV6EfQOdmKuj1cbnDQlcOp/s=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=NK/uxG+1fbV1H+MR8w7AUb4B6YWwATIJg8xOSLyWrv/uUxK1t1qAM5IinqzogB49B suZjJVuQcYpEKT+VnXPJfaIsfiOieHzg5rfsHWlu3jAaNZl0zdaK+bd+4dU7FLWV6I Ij0wD6iLLLQEUNnfroNK3DSmRHJp+zwF9u5mvRHQ= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 5F1D81E091; Thu, 30 Nov 2023 19:02:30 -0500 (EST) Message-ID: <3805f73b-5f9f-40f8-8455-b0be6951508a@simark.ca> Date: Thu, 30 Nov 2023 19:02:29 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset Content-Language: en-US To: John Baldwin , Simon Marchi , gdb-patches@sourceware.org Cc: Luis Machado References: <20231130212057.722990-1-simon.marchi@efficios.com> <20231130212057.722990-2-simon.marchi@efficios.com> <2fcb73af-37f0-40d7-aa4a-d6eac42f2ffd@FreeBSD.org> From: Simon Marchi In-Reply-To: <2fcb73af-37f0-40d7-aa4a-d6eac42f2ffd@FreeBSD.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: On 2023-11-30 18:42, John Baldwin wrote: > On 11/30/23 1:20 PM, Simon Marchi wrote: >> 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); > > This makes sense to me. In particular, note that a few lines below we do a > return for the same condition when an individual register doesn't fit. Yeah, although it could be argued that this case is different: we're targetting a single register, we found the regcache_map_entry that describes that register, so regardless of the outcome, we know we don't want to keep searching the regmap array. > Reviewed-By: John Baldwin Thanks, will add the trailer. Simon