From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id ACmVKC1ChWdgwg0AWB0awg (envelope-from ) for ; Mon, 13 Jan 2025 11:41:17 -0500 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=CfXfjN5O; dkim=pass header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=fRkHYBLJ; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=CfXfjN5O; dkim=neutral header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=fRkHYBLJ; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id A14B71E100; Mon, 13 Jan 2025 11:41:17 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-5.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=unavailable autolearn_force=no version=4.0.0 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (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 ESMTPS id DD07C1E05C for ; Mon, 13 Jan 2025 11:41:16 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 791A63858D21 for ; Mon, 13 Jan 2025 16:41:16 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 791A63858D21 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=CfXfjN5O; dkim=pass header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=fRkHYBLJ; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=CfXfjN5O; dkim=neutral header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=fRkHYBLJ Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by sourceware.org (Postfix) with ESMTPS id E97933858CDB for ; Mon, 13 Jan 2025 16:40:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E97933858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E97933858CDB Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.223.130 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736786438; cv=none; b=GqOE/2cZuA1n+8vdZuqILi8JlqXjCfE26yyoj95/uiLdcqACDRWPBEToDClfAwcvysTcTxMnocz0hPf10eLviLfmK2LU7RTZJ9ZtWpw15onuPO6GabmqoWLr7X96gYGOHS/bSGpNn44hIjg1IQvCJGbZMxeE9ovRLL4ybciBaNA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736786438; c=relaxed/simple; bh=sZuBeQUBbjihmNSAOJkZo0ey5GZYPFJZP2woxh3MfF8=; h=DKIM-Signature:DKIM-Signature:DKIM-Signature:DKIM-Signature: Message-ID:Date:MIME-Version:Subject:To:From; b=J7UrCnzZJ2BWTb/V0AvAxDgv4DqvraLR9MCZIx4YpgsG7gn2xjmf6m29I/4kp/H0ABnUmNLemmW8CwEzHLKexXXAMxk4eDr8LWaTNnLtybeTuFCWtrKsj/PJVYzzJ2s2zKO+FAGOQ1OMiNQSyRs4l5dDRSJEkxJe+Bbvf923Jrk= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E97933858CDB Received: from imap1.dmz-prg2.suse.org (unknown [10.150.64.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id A87C92116E; Mon, 13 Jan 2025 16:40:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1736786431; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=u09VygjLGcy3j/OJsrV4S9q/m+EJlkgFuGkvlkstJ2c=; b=CfXfjN5OxpD5YSPiurRYCjc5ubyPXcAFjW+ESL3tLEkVla371gdHu968QtDIvbsyzw7fgU O1XjDUPB2Y++WyZb9wMwdMneCk+NCOuoA6rVowcu4Rv1lPyhA3iKbwg+Cg/s9W4Aldp4fv Sk5EWNwSW82XgXHRLtjbiUVItwHiHRY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1736786431; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=u09VygjLGcy3j/OJsrV4S9q/m+EJlkgFuGkvlkstJ2c=; b=fRkHYBLJEOTX3fTF3ElATcN/c92Ys7JX9jSZr4quBKjKOi2UPvj/vuwGmaGQ+2noSBwzIZ i1RR4b7DiEGMh+Aw== Authentication-Results: smtp-out1.suse.de; none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1736786431; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=u09VygjLGcy3j/OJsrV4S9q/m+EJlkgFuGkvlkstJ2c=; b=CfXfjN5OxpD5YSPiurRYCjc5ubyPXcAFjW+ESL3tLEkVla371gdHu968QtDIvbsyzw7fgU O1XjDUPB2Y++WyZb9wMwdMneCk+NCOuoA6rVowcu4Rv1lPyhA3iKbwg+Cg/s9W4Aldp4fv Sk5EWNwSW82XgXHRLtjbiUVItwHiHRY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1736786431; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=u09VygjLGcy3j/OJsrV4S9q/m+EJlkgFuGkvlkstJ2c=; b=fRkHYBLJEOTX3fTF3ElATcN/c92Ys7JX9jSZr4quBKjKOi2UPvj/vuwGmaGQ+2noSBwzIZ i1RR4b7DiEGMh+Aw== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id 6FBDD13876; Mon, 13 Jan 2025 16:40:31 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id 78FOGf9BhWcyZgAAD6G6ig (envelope-from ); Mon, 13 Jan 2025 16:40:31 +0000 Message-ID: <292d5723-0fb0-485b-bf2d-02c3bf74e976@suse.de> Date: Mon, 13 Jan 2025 17:40:27 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] GDB: Use gdb::array_view for buffers used in register reading and unwinding To: Thiago Jung Bauermann Cc: Simon Marchi , gdb-patches@sourceware.org, "Aktemur, Tankut Baris" , "Maciej W. Rozycki" References: <20250110164430.3376697-1-thiago.bauermann@linaro.org> <20250110164430.3376697-3-thiago.bauermann@linaro.org> <321e71e0-43de-4604-bb7e-34f6f64b83bf@simark.ca> <871pxa9udr.fsf@linaro.org> <347ca2f6-aa4a-4f08-8675-a8f0bce65e93@suse.de> <87ikql8ibi.fsf@linaro.org> <28934663-54a6-47cf-8d30-dace9cde8d20@suse.de> <871px898i6.fsf@linaro.org> Content-Language: en-US From: Tom de Vries In-Reply-To: <871px898i6.fsf@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spamd-Result: default: False [-4.30 / 50.00]; BAYES_HAM(-3.00)[100.00%]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.20)[-0.999]; MIME_GOOD(-0.10)[text/plain]; FROM_HAS_DN(0.00)[]; FUZZY_BLOCKED(0.00)[rspamd.com]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; ARC_NA(0.00)[]; TO_DN_SOME(0.00)[]; MIME_TRACE(0.00)[0:+]; TO_MATCH_ENVRCPT_ALL(0.00)[]; RCVD_TLS_ALL(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; FROM_EQ_ENVFROM(0.00)[]; RCPT_COUNT_FIVE(0.00)[5]; RCVD_COUNT_TWO(0.00)[2]; MID_RHS_MATCH_FROM(0.00)[]; DBL_BLOCKED_OPENRESOLVER(0.00)[imap1.dmz-prg2.suse.org:helo] X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org On 1/12/25 01:32, Thiago Jung Bauermann wrote: > > Tom de Vries writes: > >> On 1/11/25 16:46, Thiago Jung Bauermann wrote: >>> Hello Tom, >>> Tom de Vries writes: >>> >>>> On 1/10/25 23:28, Thiago Jung Bauermann wrote: >>> Thank you for debugging the issue! So memcpy was overflowing the >>> buffer. Nice to see the assert in action. :) >> >> Hi Thiago, >> >> thanks for the quick review. > > Too quick, as it turns out. Looking at it again, that fix relies on > buffer.size () reflecting the correct size of the register being > unwound. Simon found a counterexample in i386_unwind_pc, which is used > for both i386 and x86_64 and thus passes an 8-byte buffer: > > https://inbox.sourceware.org/gdb-patches/321e71e0-43de-4604-bb7e-34f6f64b83bf@simark.ca/ > Ack, I've taken this into account in a patch I've just submitted ( https://sourceware.org/pipermail/gdb-patches/2025-January/214695.html ). >> Not the memcpy, but the memset, because the optimized out branch was activated, but buffer >> overflow indeed. >> I seem to have confused myself here. I thought I saw the test-case failing-but-not-asserting at 7fcdec025c0^, but I no longer see that. Instead I see it pass at 7fcdec025c0^, so it must have been an artifact of somehow not building clean or at the wrong commit. In other words, indeed the memcpy triggers a buffer overflow. My apologies for providing misleading information. >>>> Properly fixing this requires us to only look at the part that is relevant, copying the >>>> value from there, and checking for optimized out and unavailable only there. >>>> >>>> This worked for s390x-linux: >>>> ... >>>> diff --git a/gdb/frame.c b/gdb/frame.c >>>> index 10a32dcd896..02583857019 100644 >>>> --- a/gdb/frame.c >>>> +++ b/gdb/frame.c >>>> @@ -1193,8 +1193,14 @@ frame_register_unwind (const frame_info_ptr &next_frame, int >>>> regnum, >>>> >>>> gdb_assert (value != NULL); >>>> >>>> - *optimizedp = value->optimized_out (); >>>> - *unavailablep = !value->entirely_available (); >>>> + if (value->lazy ()) >>>> + value->fetch_lazy (); >>>> + >>>> + *optimizedp >>>> + = value->bits_any_optimized_out (value->offset () * 8, >>>> + buffer.size () * 8); >>>> + *unavailablep >>>> + = !value->bytes_available (value->offset (), buffer.size ()); >>>> *lvalp = value->lval (); >>>> *addrp = value->address (); >>>> if (*lvalp == lval_register) >>>> @@ -1204,13 +1210,17 @@ frame_register_unwind (const frame_info_ptr &next_frame, int >>>> regnum, >>>> >>>> if (!buffer.empty ()) >>>> { >>>> - gdb_assert (buffer.size () >= value->type ()->length ()); >>>> + gdb_assert (buffer.size () >>>> + <= value->type ()->length () - value->offset ()); >>> It should be '>=' above. >>> >> >> That doesn't work unfortunately: >> - buffer.size () is 8 >> - value->type ()->length () is 16 >> - value->offset () == 0 >> >> So we'd have gdb_assert (8 >= 16). > > Ah, I didn't know offset was 0. We do need an assert that the buffer is > big enough though. :/ > The patch I've submitted no longer has such an assert, instead it guarantees no buffer overflow through use of std::min. >> FWIW, I've tried out a less intrusive fix which also works: >> ... >> diff --git a/gdb/frame.c b/gdb/frame.c >> index 10a32dcd896..96e0752888f 100644 >> --- a/gdb/frame.c >> +++ b/gdb/frame.c >> @@ -1191,6 +1191,19 @@ frame_register_unwind (const frame_info_ptr &next_frame, int >> regnum, >> >> value = frame_unwind_register_value (next_frame, regnum); > > Your fix below is interesting, but I'm starting to think that the real > bug is that frame_unwind_register_value returns a value with a type that > isn't regnum's type. Even if the value was found in another register, > that shouldn't concern the caller. WDYT? > I'm not sure. In this case the type seems less relevant because it's not returned from frame_register_unwind. What is returned is value->regnum (), so this size discrepancy could propagate. Things would be greatly simplified if frame_unwind_register_value returned the value of f0 instead of v0 in this case. But perhaps that is a special case, and we're better off covering the generic case here in frame_register_unwind and elsewhere. FWIW, I'm working on other patches addressing the exact same problem in other places. > Also, should I revert the patch while we don't find a solution? > If my proposed patch is acceptable, then there's no need, otherwise we might need to revisit that. >> + frame_info_ptr this_frame = get_prev_frame (next_frame); >> + struct gdbarch *gdbarch = frame_unwind_arch (this_frame); > > Shouldn't this be either: > > struct gdbarch *gdbarch = frame_unwind_arch (next_frame); > > or: > > struct gdbarch *gdbarch = get_frame_arch (this_frame); > > ? I still get confused about this/next in frame unwinding... > Yeah, I was and very much still am confused about this. In my proposed patch, that's no longer relevant, I've pretty much reverted to my original approach. >> + size_t reg_size = register_size (gdbarch, regnum); >> + >> + if (value->type ()->length () > reg_size) >> + { >> + struct value *part_val >> + = value::allocate_register (this_frame, regnum); > > Shouldn't next_frame be used here? (Same confused face as above.) > >> + value->contents_copy (part_val, 0, value->offset (), reg_size); >> + release_value (value); >> + value = part_val; >> + } >> + >> gdb_assert (value != NULL); >> >> *optimizedp = value->optimized_out (); >> ... >> but I have doubts whether reg_size is correct in case gdbarch changes between frames and >> register sizes are different. > > AFAIU GDB should know the gdbarch of each frame in the stack, otherwise > things can get weird. Also, is there another option? As I mentioned > before, the buffer size can't be used unless we audit callers to make > sure they pass a buffer with the same size as the register being unwound > (as Simon mentioned in the email I linked above). I'm not sure I understand your question, but my concern is that I'm not picking the right frame. Picking the wrong frame will only matter in case architecture changes between frames, OTOH detecting that we've picked the wrong frame means exercising that scenario, and I don't know how to. Thanks, - Tom