From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x232.google.com (mail-oi1-x232.google.com [IPv6:2607:f8b0:4864:20::232]) by sourceware.org (Postfix) with ESMTPS id A9A953858D32 for ; Mon, 29 May 2023 17:31:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A9A953858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oi1-x232.google.com with SMTP id 5614622812f47-397f6a71ee7so2403389b6e.3 for ; Mon, 29 May 2023 10:31:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685381489; x=1687973489; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=94P0MocvHSRWfwTLKzqj13lSChrqiXJteGfhOHIAu+0=; b=pxtFLMSZ9NjGZqRPYn9clnKiBPwnIx0lswKLDxyiD0TLrpwExPuTsNU2d4A968uKad n3P2VUTiTC9C+pUWgv5oiNjVzqRjzqpFvAVzcK6K/1KCoDOvGD3yaYvWiIecBfKx+RXP IR4hciwK2AAgfuaJwiokRbEnwsW70mQw2aKXAOnR+ip3g84rHQ4navQYVRSNuqJ7Jn0x INqqkYWLXb72eGdFJ/taYlRP2XCDx+i8dLggTZnPQVrv3MAY5KpsjzqYTQD+clDkTsUy FPuZ2/8+WzS7gLVzfwW1bz3dQUyzLb/o2k9E44ndOcfGNDWFMNEBYxhb0T9oRxqFPycK LbzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685381489; x=1687973489; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=94P0MocvHSRWfwTLKzqj13lSChrqiXJteGfhOHIAu+0=; b=ApsUHSk0VYZg67Vg6uz1F9bOYYnb79EH7z8NgFm15ST4oeFEv40cOvmlvJewXxEx3M 4BTgeD4lsyh0+vM4A8dUpMJlNm6IF9zTUVak8x5p0sjfG03hxcFjRK4gFfmA0etxWbov 8yVp/sCpFkJZ1FLHs43seFXJcROFctOXwLz86sGV/Ss2VxOsBrXGvtHVKTg8+CAA9mj1 FTjI1uI6KZD7V2tkoM5jYqjLtbt28X8hCxiMk9ZQpGrZN8YORFgFQ/79Q715KPevmChB +lWG8XfLpgWgSFMVZW6EIjMIe8gfPS2Nk1uoslqMHZAltasaMhi+nEK2V1Q6xQh9JPwT ZaRw== X-Gm-Message-State: AC+VfDwl1EYGpnEAnzOSpCP56a4HAkoibdGlnS86zc9vbsK2SWG4+7Oh a8kpY1hK3+9GJzshY8JaNn2kX7qvKGGk4TrjTsmaSPBjmGI= X-Google-Smtp-Source: ACHHUZ6WxlIt+cpBphSkiZ5lMnedkwSKD2835rkOFXsuWwIQ5VnJITRMQJRyPFXkbVzBmKqBIeberD9dM6osyLjrM3c= X-Received: by 2002:a05:6808:6c8:b0:398:1027:4fae with SMTP id m8-20020a05680806c800b0039810274faemr5712427oih.25.1685381488714; Mon, 29 May 2023 10:31:28 -0700 (PDT) MIME-Version: 1.0 References: <20230528172013.73111-1-bugaevc@gmail.com> <20230528172013.73111-4-bugaevc@gmail.com> <31457dbb-a805-262f-4b62-be0b40960ca6@linaro.org> In-Reply-To: <31457dbb-a805-262f-4b62-be0b40960ca6@linaro.org> From: Sergey Bugaev Date: Mon, 29 May 2023 20:31:17 +0300 Message-ID: Subject: Re: [PATCH v2 3/3] io: Add FORTIFY_SOURCE check for fcntl arguments To: Adhemerval Zanella Netto Cc: libc-alpha@sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,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: Hello, On Mon, May 29, 2023 at 7:54=E2=80=AFPM Adhemerval Zanella Netto wrote: > Some comments below. thank you for the review :) > > +/* Return VALUE, but do it in a way that the compiler cannot > > + see that it's a compile-time constant. */ > > +static int __attribute_noinline__ > > +hide_constant (int value) > > +{ > > + volatile int v =3D value; > > + return v; > > +} > > + > > Interesting construct, but I wonder if 'volatile' is really required here= . > Afaiu what really prevents compiler to return fortify compiler warning he= re > is the __attribute_noinline__. It's not really required. Apparently even an 'inline' function call is enough to to disarm __builtin_constant_p [0]; I just threw both volatile and noinline in for good measure. [0] https://godbolt.org/z/v9569KGTT > > + struct flock flock; > > Maybe move the flock struct to within the block for where ofd_locks_suppo= rted > is true? It's used in the following if (ofd_locks_supported) block -- the one checking for non-compile-time-const cmd -- too. > > +#if defined (__USE_LARGEFILE64) || defined (__USE_TIME_BITS64) > > There is no need to replicate the tests for the LFS support, this is alre= ady > done by tst-fortify-cc-lfs-X tests. Hm, I don't think I understand your point, could you please expand? The -lfs- tests are the ones that enable -D _FILE_OFFSET_BITS=3D64, right? Here, I'm testing that if __USE_LARGEFILE64 is enabled (i.e. -D _LARGEFILE64_SOURCE), which means fcntl64 () is available as a separate function, the fcntl64 fortification also behaves as expected. It's not enough to just test that fcntl () works as expected in 64-bit mode, we need to test that fcntl64 () also works as expected. These are separate functions (perhaps aliases of each other, but separate fortified macros for sure) that may or may not both work with 64-bit types. __USE_LARGEFILE64 and __USE_FILE_OFFSET64 are orthogonal in my understanding, aren't they? __USE_LARGEFILE64 means that xxxx64 APIs are declared as a separate set of APIs that explicitly work with 64-bit types; __USE_FILE_OFFSET64 means that the regular APIs (xxxx, not xxxx64) are declared to already use 64-bit. You can have neither enabled, or one of them enabled, or both. No? > > +template > > +struct __fcntl_types_compatible_helper > > This makes the C++ tests fail with GCC 13: > > [...] > ../include/bits/../../io/bits/fcntl2.h:460:1: error: template with C link= age > 460 | template > | ^~~~~~~~ > ../misc/sys/cdefs.h:140:25: note: =E2=80=98extern "C"=E2=80=99 linkage st= arted here > 140 | # define __BEGIN_DECLS extern "C" { Interesting. Any idea why this wasn't failing in my testing? > You will will need to include bits/fcntl2.h after the __END_DECLS, and ad= d > __BEGIN_DECLS/__END_DECLS on fcntl2.h. Something like this on top this > patch Right, makes sense, thank you. > I think we will need to enable fcntl fortify only for gcc 8 or higher, si= nce > __VA_OPT__ is not support on gcc 7. So maybe it is a good idea to move this all into fcntl3.h like it was done in Florian's patch? Then I'd include that after __END_DECLS and only if GCC >=3D 8. Sergey