From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 4BtSINkNg2dbKwsAWB0awg (envelope-from ) for ; Sat, 11 Jan 2025 19:33:29 -0500 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=uwPDozEw; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 810151E0C0; Sat, 11 Jan 2025 19:33:29 -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 0E5F81E05C for ; Sat, 11 Jan 2025 19:33:29 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9E462385802C for ; Sun, 12 Jan 2025 00:33:28 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9E462385802C Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=uwPDozEw Received: from mail-ot1-x335.google.com (mail-ot1-x335.google.com [IPv6:2607:f8b0:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id B19C73858D3C for ; Sun, 12 Jan 2025 00:32:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B19C73858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B19C73858D3C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::335 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736641974; cv=none; b=tv830NAUmXfKetDJDHsUUSYZbhH+NmzTPNXNKzmeUaIDV4Rp8XIIFnL6SDl3Y6KWpsLqohPmqWYOjElDRj0O5zlcC7ELWZtcmEARymkn9kOBUC5vr3jU2U2hJwPn+PKZ1GfHJUNfmW1HR+fs8Yc1sSf5+H0+oaPGGFQ/81CqRuo= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1736641974; c=relaxed/simple; bh=GO1ZNTM3lY3FSCIXwN2em8188A36lzI19/7kzZx/p5U=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=Fk38DOYv62taOhIpNSDWRQ41gedRzEh3GJLtA8WVbgFzdkaT5XX6byZcKfJGe9hxcMfCdq/ANMpBDxLWEFOPt2sSpVYrIiONanyPDMDLmTEtzUmiUQcfF99Iw506bHYIdTTjLk9Y8n1Ahuwb5ZybgtmW9Y5sIl6sZeYa3g03zzA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B19C73858D3C Received: by mail-ot1-x335.google.com with SMTP id 46e09a7af769-71e3eb8d224so820657a34.2 for ; Sat, 11 Jan 2025 16:32:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1736641974; x=1737246774; darn=sourceware.org; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:from:to:cc:subject:date:message-id:reply-to; bh=Y8tIXnoFH8qVdSbOa7FPzUc/5qnUPtlXwS0mhqgi0cs=; b=uwPDozEw5eqdHhxCqtqUN2cxd3BxpnjmORIjw25f+CQGN9HTmiHzVEktZ3Gwelc+WH jsgq9mo7yqxwIuTA/ZIILSIFOFOIu4EnKAf2txI0GiMTm+LikMegAt4aXPRt3w0HUAjN SaBL3UwobZdxygSrcFYn+X/av0ji/z5f1VmTVejPNlB/dHWJ1DCOi9pC2Awe7TUM6hMh MTwHCBBWGt1v3PMG6r0BjR6zYjxGCZ07UV0qGEp6Xv5Ph3HxkPAYbzOA5Tm9aoI6wzwP WAKc+85nNvCYWmKxhKGXKjW2uQ9GRy3zcQRzkKeq64wp+b7v4vs6y9LFT7/mg25h+R/u V8hQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736641974; x=1737246774; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Y8tIXnoFH8qVdSbOa7FPzUc/5qnUPtlXwS0mhqgi0cs=; b=C7xqhj6w0/+rnIDSrtdL7wE203vS28zIj1Z1KObkokRsb5jGI9wkNjfKWLsMkwFU2x 2fSfwGMmZHzsieWmaWxb9HLMFqe/qX9cmPYJtWyUGC7ozFdE/a0esHBcIk4DJAbC1G4M sGfmtPCMoU5mhNWTAQA2y2zR5SFgsdiJJXGCV9Ly/MGEC2agB6er3NrMsOXxpXBPq6uu 6CwWecO3huk7wMT1f5wBQU9PH4IznWynft56FkzV6JmJO9yVW63lO1E3wB3patDj7IhX IIhjRleBPN8I4+uTG4sJirK3kMX6K77hActKHbkddMXDCZs56AClfpkwDwt0+hpSA5D8 HQiw== X-Forwarded-Encrypted: i=1; AJvYcCWZ57FHR8W1GBMUfiqnUMpSoF0crriwX69wAHnUd+YFJsb7AcG+cLchbb6DLyBbmsl1NxNyHCP2HFgwiQ==@sourceware.org X-Gm-Message-State: AOJu0YyqFcxrWIttIzl7i96xl7H2eDBHqGXCdcYrkfXqQYGTTMwxYzNB 61Fak1vaZQ0FxYXEzbjVcJaRE5cE3UQ4jKvPqk7pAFP2bSuavrFcMCBz/Xoe74g= X-Gm-Gg: ASbGnct5+K2lBZ6wbMY0peHfPx9IMm6wO0os0Y1/LFbgeflwPtw4Q3dGwiM/Z7rVtJv ZELTMPbXsp8L+ELpsn9tLPC/aBp4ryVnzl6N3ytzD/qvG87VGI/v6hx9K4B8FdmCdEYElDZOvEQ 0yVuuOI28BfDbri2L3d6FrBBJryk0aQWxRJuKjq2Ogsq4X4Ngl8Bt2JJFwOL+6fk7E/IUyD1bil eFA7U7ZrHETRc+UTGERf8lOCkWX6uWMDSo2xt/Py7o+qRBuCqn7AHu7FgQwBZ/qlc0= X-Google-Smtp-Source: AGHT+IHVaJp8Fd6HA1t4rg/xgtJsv6mQJrla+/j60sAyruyD26M74RgL2YDLFuIj2QRHnLq0grLWRQ== X-Received: by 2002:a05:6870:9c8f:b0:29e:5e83:150e with SMTP id 586e51a60fabf-2aa069a90e2mr8839292fac.27.1736641973956; Sat, 11 Jan 2025 16:32:53 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:4617:65aa:26fe:5ce1]) by smtp.gmail.com with ESMTPSA id 586e51a60fabf-2ad809a759dsm2423327fac.33.2025.01.11.16.32.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 11 Jan 2025 16:32:52 -0800 (PST) From: Thiago Jung Bauermann To: Tom de Vries Cc: Simon Marchi , gdb-patches@sourceware.org, "Aktemur, Tankut Baris" , "Maciej W. Rozycki" Subject: Re: [PATCH 2/2] GDB: Use gdb::array_view for buffers used in register reading and unwinding In-Reply-To: <28934663-54a6-47cf-8d30-dace9cde8d20@suse.de> (Tom de Vries's message of "Sat, 11 Jan 2025 17:01:37 +0100") 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> User-Agent: mu4e 1.12.8; emacs 29.4 Date: Sat, 11 Jan 2025 21:32:49 -0300 Message-ID: <871px898i6.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain 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 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/ > Not the memcpy, but the memset, because the optimized out branch was activated, but buffer > overflow indeed. > >>> 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. :/ > 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? Also, should I revert the patch while we don't find a solution? > + 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... > + 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). -- Thiago