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 C25ED3857C52 for ; Thu, 14 Mar 2024 17:52:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C25ED3857C52 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 C25ED3857C52 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=1710438771; cv=none; b=c3mE7gDwxyYa4kjcU1mgWZ6kuVp3M2UiHDZdybfHAHMF7bGJn+QE0I1GY8MSF6S7mgT/1S2v8+dUeNDyQTuMrb71+YdGy1q3G9u1uqCmfowSs7UR7PoPDNx1HkLK1TSeKiv1yb0k8yJM10zaDPEDVvh2m/int43He/Kbb0lEHpc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710438771; c=relaxed/simple; bh=HVPAGbczWtoE6MkI7eDCaEEJPo/T9iF8Dl5HAMVq0iE=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=XsW/NA/FbXEHlXeFSUFoVTpnv7wlscJyMpMtq/bPc+DYQ0NcUxf+9xMfGUqnrou2pcjSfqNAUkV0AqUzN1sScA85bpXrM2kF/t7YdLM3nsYNWx/LvC0VQVccVCrWG31y3EQqtd6sssmz/ANwWSRyngHo3/1SOYTtEdbkAdC5hfc= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1710438769; bh=HVPAGbczWtoE6MkI7eDCaEEJPo/T9iF8Dl5HAMVq0iE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=kDlBeRMVPA9yXxur7JhI+wZWcJzsj0HNSzaRjJ0jV+hSsxtc5Fx82YipCPEty8qQk y0xPX53sn8pV1TCcl28H/aihmyVA+vFqKCeyxBSIVVv/Ch0q4b3P4OcGnP/ADqLmkk Gn8uAZC4xLU6GB9HjJblpeFfCDad6WIJfqoybITQ= 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 282521E01D; Thu, 14 Mar 2024 13:52:49 -0400 (EDT) Message-ID: Date: Thu, 14 Mar 2024 13:52:48 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] gdb/dwarf2: Check for missing abbrev Content-Language: en-US To: Aaron Merey , Tom Tromey Cc: gdb-patches@sourceware.org References: <20240313201827.1853989-1-amerey@redhat.com> <878r2lm1un.fsf@tromey.com> <22883d8d-b634-4e71-802c-239114d01279@simark.ca> <87r0gclw9w.fsf@tromey.com> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 2024-03-14 13:45, Aaron Merey wrote: > On Wed, Mar 13, 2024 at 9:47 PM Simon Marchi wrote: >> >> We generally don't make functions error out like this if they are passed >> bad parameters (we can use gdb_assert for that if we really want to). >> It's up to the callers to respect the contract and not pass nullptr >> where nullptr is not expected. So I would instead suggest to add the >> nullptr check in the caller (in scan_attributes itself, after the >> peek_die_abbrev call lower). > > Moving the nullptr check after peek_die_abbrev is fine, but replacing > the throw with 'return nullptr' doesn't fix the crash. It looks like > scan_attributes currently cannot return nullptr without triggering > a crash. I was not suggesting to make scan_attributes return nullptr, throwing an error (with error() as Tom pointed out it's fine). I was just suggesting doing it in the caller rather than in the callee. >> However, I'd like if we could analyze the problem a bit further to >> understand more precisely what happens, just to be sure that there isn't >> a more fundamental problem and we're not just papering over the problem. >> I added instructions on the bug that should help you reproduce. >> >> What I see is that read_unsigned_leb128 in peek_die_abbrev unexpectedly >> returns 0. Is it because the corrupted file contains zeros where it >> shouldn't? Or is the file truncated and we are reading past the end of >> the buffer, and there happens to be a zero there? > > The corrupt debuginfo that the reporter used to reproduce this [1] is the > same size as the non-corrupt version. A diff of hex dumps of the > corrupt and non-corrupt versions of the debuginfo show that the corrupt > file differs by containing all zeros in some places. Thanks for checking! > On Thu, Mar 14, 2024 at 8:36 AM Tom Tromey wrote: >> >> Also, I was wondering if this case can be tested somehow. Perhaps the >> DWARF assembler could be modified to allow the creation of corrupted >> debug info. It seems to me if we're going forward with the security >> policy, then we're going to need to test these things. > > We should be able to reproduce this in a testcase by overwriting part of > a small binary with zeros. I'll resubmit the patch with a testcase. It sounds difficult to produce something that will give a reliable result. But if you can get it working, that's cool. In the back of my mind, I always think that we should have a repo or archive on the side (to avoid bloating the source repository) with sample binaries like this one. Crafting a test binary with the DWARF assembler has its advantages, but so is trying it on the real thing. We could have tests in the testsuite that would run only if you have the binary archive repo available locally. Simon