From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dog.elm.relay.mailchannels.net (dog.elm.relay.mailchannels.net [23.83.212.48]) by sourceware.org (Postfix) with ESMTPS id 7E9FC3858D33 for ; Thu, 28 Jul 2022 17:27:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7E9FC3858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id EB436C1818; Thu, 28 Jul 2022 17:27:12 +0000 (UTC) Received: from pdx1-sub0-mail-a307.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 17BB7C17D9; Thu, 28 Jul 2022 17:27:12 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1659029232; a=rsa-sha256; cv=none; b=EzHV2zGcYNrhY3+KLFHFm8nqJiqXQ40/5TJgqujK2nRORL9Tp7k9+rQth6CHp0GfHfyawj 8Z3OIIWoQMUjbogXPAH4gmGu68B3veCJ+oy2+Mf2RJZkJWK4aVZfXji57HBw/ExcmJKRhB vfSLmfHOCIq32nzQUPjndTPEQgxvtAFFf6QzJcidWxAqAl22M1Ev8FlqWi81FXxa9C1y0M 9hSYCmVelZ4HDIwfdcD6LupVv/UtSN2oKjOrQEqbQcXiGsDSyu0rNA5TFIZM5C6DLPVmDL 4LoPLg38qM+lumuq2aYK8lEoLEWcvop0IF5TO0OeypudOG4D5HbMatp66wE0Xw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1659029232; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=DSnYkLQlRSGUY9U7bhMT0HfVGeDyT1H2lnpqIKRy2+8=; b=oBtHM0A+gGWryiS5EQw0NpT4ksQt1bEjAHbQEMJ+UxQmp4yyDCPq5hrRViG0HjOFOrKJgj +/RTbKL6lencb/CA1wUY1kKLkVIuqaWbny4ZqVA9Bk1ixo+HnKAW5NmZgcaddm3TDv4Gh8 tpBEyFgNBvyJJDOBMpRs9qnCMcNqhxfZenV2IRNEAi6IQB3qeI7hGxjtHgoXaE6PR9Aoe/ s+T4/wADPfMNE6TtdIlWNw61Mlws1ni7OEuBn4sbuGIgcleBcNc9RGZFxuvxo02czjoG4f 6rCx8+eVSXq3ai6BwyBoI5i3uTGOqA52JLgYnM2BPCfDZFDaT2JhHONzCBDHlg== ARC-Authentication-Results: i=1; rspamd-677d99db4d-qp8fl; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Cure-Tart: 5bfb3331019e6603_1659029232364_1230011458 X-MC-Loop-Signature: 1659029232363:420364075 X-MC-Ingress-Time: 1659029232363 Received: from pdx1-sub0-mail-a307.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.115.45.60 (trex/6.7.1); Thu, 28 Jul 2022 17:27:12 +0000 Received: from [192.168.2.151] (bras-base-bmtnon1328w-grc-12-174-91-14-188.dsl.bell.ca [174.91.14.188]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a307.dreamhost.com (Postfix) with ESMTPSA id 4LtyJH3jkJz2v; Thu, 28 Jul 2022 10:27:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gotplt.org; s=dreamhost; t=1659029231; bh=DSnYkLQlRSGUY9U7bhMT0HfVGeDyT1H2lnpqIKRy2+8=; h=Date:Subject:To:From:Content-Type:Content-Transfer-Encoding; b=oijehnu1JfmMSPA59ceRvuI+wQ9B1tjW7cMYvKCcznawnTSIbT4OGh/LjQsWkk5qS Qa0Fso2DXLkKRnN0LwtPk1idNa5rEPwyJWN8K3O+GW7XS0q5dwj4vvS7idNgEmf/Lp F4dqpT96Zfa7weapxk3Z+LFPCD+/wuNYOaWlNmlt4hAcCTk2h7hvbnKEZjW23Qr2FS xWRIah6hyDg4B+cxUVAIMA7rpyWSFs9OSgTsT60RaR2iNq2vCoXN9cudjdhUTOCoZC 84YAnl1tJZVxkehYzihlPrNo4bK390I2+ohOVpA6rBtad/N53Np4GBaIp+hZUI/EiQ qHXh0o5mg5sTA== Message-ID: Date: Thu, 28 Jul 2022 13:27:10 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] libdwfl: Rewrite reading of ar_size in elf_begin_rand Content-Language: en-US To: Mark Wielaard , elfutils-devel@sourceware.org References: <20220728134844.8618-1-mark@klomp.org> From: Siddhesh Poyarekar In-Reply-To: <20220728134844.8618-1-mark@klomp.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3038.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Jul 2022 17:27:16 -0000 On 2022-07-28 09:48, Mark Wielaard wrote: > With GCC 12.1.1, glibc 2.3a, -fsanitize=undefined and > -D_FORTIFY_SOURCE=3 we get the following error message: > > In file included from /usr/include/ar.h:22, > from ../libelf/libelfP.h:33, > from core-file.c:31: > In function ‘pread’, > inlined from ‘pread_retry’ at ../lib/system.h:188:21, > inlined from ‘elf_begin_rand’ at core-file.c:86:16, > inlined from ‘core_file_read_eagerly’ at core-file.c:205:15: > /usr/include/bits/unistd.h:74:10: error: ‘__pread_alias’ writing 58 or more bytes into a region of size 10 overflows the destination [-Werror=stringop-overflow=] > 74 | return __glibc_fortify (pread, __nbytes, sizeof (char), > | ^~~~~~~~~~~~~~~ > /usr/include/ar.h: In function ‘core_file_read_eagerly’: > /usr/include/ar.h:41:10: note: destination object ‘ar_size’ of size 10 > 41 | char ar_size[10]; /* File size, in ASCII decimal. */ > | ^~~~~~~ > /usr/include/bits/unistd.h:50:16: note: in a call to function ‘__pread_alias’ declared with attribute ‘access (write_only, 2, 3)’ > 50 | extern ssize_t __REDIRECT (__pread_alias, > | ^~~~~~~~~~ > cc1: all warnings being treated as errors > > The warning disappears when dropping either -fsanitize=undefined > or when using -D_FORTIFY_SOURCE=2. It looks like a false positive. > But I haven't figured out how/why it happens. Interesting, I'll take a closer look at this from the gcc context. I obviously don't have any strong opinions about the elfutils patch :) Thanks, Sid > The code is a little tricky to proof correct though. The ar_size > field is a not-zero terminated string ASCII decimal, right-paddedr > with spaces. Which is then converted with strtoll. Relying on the > fact that the struct ar_hdr is zero initialized, so there will be > a zero byte after the ar_size field. > > Rewrite the code to just use a zero byte terminated char array. > Which is much easier to reason about. As a bonus the error disappears. > > Signed-off-by: Mark Wielaard > --- > libdwfl/ChangeLog | 5 +++++ > libdwfl/core-file.c | 26 ++++++++++++++++---------- > 2 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog > index 75c53948..acdaa013 100644 > --- a/libdwfl/ChangeLog > +++ b/libdwfl/ChangeLog > @@ -1,3 +1,8 @@ > +2022-07-28 Mark Wielaard > + > + * core-file.c (elf_begin_rand): Replace struct ar_hdr h with > + a char ar_size[AR_SIZE_CHARS + 1] array to read size. > + > 2022-07-18 Shahab Vahedi > > * debuginfod-client.c (dwfl_get_debuginfod_client stub): > diff --git a/libdwfl/core-file.c b/libdwfl/core-file.c > index cefc3db0..4418ef33 100644 > --- a/libdwfl/core-file.c > +++ b/libdwfl/core-file.c > @@ -75,26 +75,32 @@ elf_begin_rand (Elf *parent, off_t offset, off_t size, off_t *next) > from the archive header to override SIZE. */ > if (parent->kind == ELF_K_AR) > { > - struct ar_hdr h = { .ar_size = "" }; > - > - if (unlikely (parent->maximum_size - offset < sizeof h)) > + /* File size, in ASCII decimal, right-padded with ASCII spaces. > + Max 10 characters. Not zero terminated. So make this ar_size > + array one larger and explicitly zero terminate it. As needed > + for strtoll. */ > + #define AR_SIZE_CHARS 10 > + char ar_size[AR_SIZE_CHARS + 1]; > + ar_size[AR_SIZE_CHARS] = '\0'; > + > + if (unlikely (parent->maximum_size - offset < sizeof (struct ar_hdr))) > return fail (ELF_E_RANGE); > > if (parent->map_address != NULL) > - memcpy (h.ar_size, parent->map_address + parent->start_offset + offset, > - sizeof h.ar_size); > + memcpy (ar_size, parent->map_address + parent->start_offset + offset, > + AR_SIZE_CHARS); > else if (unlikely (pread_retry (parent->fildes, > - h.ar_size, sizeof (h.ar_size), > + ar_size, AR_SIZE_CHARS, > parent->start_offset + offset > + offsetof (struct ar_hdr, ar_size)) > - != sizeof (h.ar_size))) > + != AR_SIZE_CHARS)) > return fail (ELF_E_READ_ERROR); > > - offset += sizeof h; > + offset += sizeof (struct ar_hdr); > > char *endp; > - size = strtoll (h.ar_size, &endp, 10); > - if (unlikely (endp == h.ar_size) > + size = strtoll (ar_size, &endp, 10); > + if (unlikely (endp == ar_size) > || unlikely ((off_t) parent->maximum_size - offset < size)) > return fail (ELF_E_INVALID_ARCHIVE); > }