From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x332.google.com (mail-ot1-x332.google.com [IPv6:2607:f8b0:4864:20::332]) by sourceware.org (Postfix) with ESMTPS id 5A499385840C for ; Mon, 29 May 2023 20:14:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5A499385840C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-ot1-x332.google.com with SMTP id 46e09a7af769-6af6df7f93aso2806310a34.0 for ; Mon, 29 May 2023 13:14:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1685391252; x=1687983252; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=jTjiENb2vmqhjl+yWaVOdufQIn5U1pXbm48aKmk18Y8=; b=EiYMGX7GWdHyk4fS/lMY8tTRy38EbePVEiNkGsCJZvGZsduKv3jI8d0CkWQ0T8zt4C qXmmNfwqEPnq0yJnwlGP6HuqBPrrdErmtpCixfClqof6FScC7+jmi8x1shXsPXXOQ23P Lpim3rF4tuP9zhs5sS/PUt8yhjEYtyO/MhYFxVaIJ3FLpM67nZj/rOzV0kqPK+vcqMVB yNaow6uTtzE58VbSJYo8iyDnkkGRf/A653IVhC7mTDtf7EtyjP4x8Jb1I1EumyXhPhg1 KMVAxqbMkNdPHysFMldPi3D6PqlIB+ZutPdYNYbAq+TxIzkDh+1ofG8vig+6PQUz8GD9 bgTg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685391252; x=1687983252; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=jTjiENb2vmqhjl+yWaVOdufQIn5U1pXbm48aKmk18Y8=; b=R6oyGxVc1wqrFb6ulMNmCBlw5EAX2+vc6hTgm3ClGLYbcW2+9kQhDV9f0lGBT3LqQa C2OWWUvrxgPErM8/7+MguDG/jfc7+r0BtGZbnWhqkUeO0+qSPnpirTaym5HlVmxNWF+1 dxkX0hiQ65aw2+4mMH+XPP1vy52ThAhMRJprRT7f8htiUZo8GmsLXahHVLFp84mVg319 9v40ZSRp33ftV5nFnQKf58yFYH/2JnJWo82wIXAiX5IvdKFVq4HwgI2S/a7UNAVY4ugF qJTqIPEolC9bDPKoqGonWwk9Ky9wrQXwfs2q+lbY022SLbMYIIWUvpT+wIZpUAxZYaP3 RQXw== X-Gm-Message-State: AC+VfDwWpBq7WHlVHlFCjhvrEvHW4PY7lQOrPwWNx+WkcOxBQ13iZN39 sj4/Bbf/j/ag47q9t0EwvpjRmQsqZlyR02RhWWvOmw== X-Google-Smtp-Source: ACHHUZ4VRSCyHo1zMTWKBZdpgja4ciztRK4FNTbH0ZoD+CsESRRblk9uS09jXbT2V1OO4qfV6/hN1A== X-Received: by 2002:a9d:6553:0:b0:6af:9f77:54c7 with SMTP id q19-20020a9d6553000000b006af9f7754c7mr28633otl.9.1685391252532; Mon, 29 May 2023 13:14:12 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c1:4dd5:b058:c94a:90a7:2c43? ([2804:1b3:a7c1:4dd5:b058:c94a:90a7:2c43]) by smtp.gmail.com with ESMTPSA id r14-20020a9d750e000000b006ade3815527sm4910345otk.22.2023.05.29.13.14.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 29 May 2023 13:14:11 -0700 (PDT) Message-ID: Date: Mon, 29 May 2023 17:14:09 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.11.1 Subject: Re: [PATCH v2 3/3] io: Add FORTIFY_SOURCE check for fcntl arguments Content-Language: en-US To: Sergey Bugaev Cc: Florian Weimer , libc-alpha@sourceware.org References: <20230528172013.73111-1-bugaevc@gmail.com> <20230528172013.73111-4-bugaevc@gmail.com> <31457dbb-a805-262f-4b62-be0b40960ca6@linaro.org> <8354c659-cfb0-993a-2764-72a2cd6f6ed4@linaro.org> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,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: On 29/05/23 16:57, Sergey Bugaev wrote: > On Mon, May 29, 2023 at 9:09=E2=80=AFPM Adhemerval Zanella Netto > wrote: >>> It's not really required. Apparently even an 'inline' function call i= s >>> 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 >> >> It is because afaiu 'volatile' does not really match the semantics of = what you >> are trying to do here. Maybe if you really need the compile to force = lvalue >> evaluation, it should use something similar to math_opt_barrier. >=20 > So, an asm block that does nothing, but is said to read and modify the > value? Sounds better indeed, and reminds me of how > std::hint::black_box is implemented. Now that you brought Rust black_box, we already something similar on benchtests: DO_NOT_OPTIMIZE_OUT. >=20 >> Nevermind, you do need to check for __USE_LARGEFILE64 symbols. But >> you can omit the '#if defined", they are always defined. >=20 > Uhh that's unfortunate actually. I hoped the test would check the > non-__USE_LARGEFILE64 configuration as well, to catch some > __USE_LARGEFILE64-only definition being used unconditionally, for > example. >=20 The LFS names are not considered a namespace pollution, so I think that's= why it always provided (just check tst-fortify.c LFS name usage, like pread64). >> And debugging a bit, it seems that I found another issue. Building >> tst-fortify-c-lfs-1-def.c the post-processed output (-E -dD) for fcntl= call is: >> >> res =3D (0 ? __fcntl_2_inline (0, 2) : !__builtin_constant_p (2) ? __f= cntl_alias (0, 2, 0) : (__fcntl_is_int (2) ? __builtin_types_compatible_p= (__typeof (0), int) : __fcntl_is_const_uint64_t_ptr (2) ? (__builtin_typ= es_compatible_p (__typeof (0), const __uint64_t *) || __builtin_types_com= patible_p (__typeof (0), __uint64_t *)) : __fcntl_is_uint64_t_ptr (2) ? _= _builtin_types_compatible_p (__typeof (0), __uint64_t *) : __fcntl_is_con= st_fowner_ex (2) ? (__builtin_types_compatible_p (__typeof (0), const str= uct f_owner_ex *) || __builtin_types_compatible_p (__typeof (0), struct f= _owner_ex *)) : __fcntl_is_fowner_ex (2) ? __builtin_types_compatible_p (= __typeof (0), struct f_owner_ex *) : __fcntl_is_const_flock (2, 0) ? (__b= uiltin_types_compatible_p (__typeof (0), const struct flock *) || __built= in_types_compatible_p (__typeof (0), struct flock *)) : __fcntl_is_flock = (2, 0) ? __builtin_types_compatible_p (__typeof (0), struct flock *) : __= fcntl_is_const_flock64 (2, 0) ? (__builtin_types_compatible_p (__typeof (= 0), const struct flock64 *) || __builtin_types_compatible_p (__typeof (0)= , struct flock64 *)) : __fcntl_is_flock64 (2, 0) ? __builtin_types_compat= ible_p (__typeof (0), struct flock64 *) : 1) ? __fcntl_alias (0, 2, 0) : = __fcntl_warn (0, 2, 0)); >> >> Which is the non-LFS version. >=20 > Hmm, I don't see the issue? And the test passes, doesn't it? :) > (If it passes *despite* the 32-vs-64 confusion, that is a problem in > itself, that'd mean it's not checking what it's supposed to be > checking.) >=20 > The fcntl macro will always expand to the same thing (modulo C vs C++ > type check differences). But then __fcntl_alias and __fcntl_warn both > reference the same symbol that fcntl would reference had it not been > for the fortification -- which is going to be the 'fcntl64' symbol > with __USE_FILE_OFFSET64. >=20 >> So I think this patch should do the same: >> >> #ifndef __USE_FILE_OFFSET64 >> # define fcntl(fd, cmd, ...) = \ >> (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd) = \ >> __VA_OPT__ (: = \ >> !__builtin_constant_p (cmd) ? __fcntl_alias (fd, cmd, __VA_ARGS__= ) \ >> : __fcntl_type_check (cmd, __VA_ARGS__, 0) = \ >> ? __fcntl_alias (fd, cmd, __VA_ARGS__) = \ >> : __fcntl_warn (fd, cmd, __VA_ARGS__))) >> #else >> # define fcntl(fd, cmd, ...) = \ >> (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd) = \ >> __VA_OPT__ (: = \ >> !__builtin_constant_p (cmd) ? __fcntl64_alias (fd, cmd, __VA_ARGS= __) \ >> : __fcntl_type_check (cmd, __VA_ARGS__, 0) = \ >> ? __fcntl64_alias (fd, cmd, __VA_ARGS__) = \ >> : __fcntl64_warn (fd, cmd, __VA_ARGS__))) >> #endif >=20 > What's the point? -- __fcntl_alias and __fcntl64_alias are supposed to > both reference the same thing (fcntl64 or __fcntl_time64) with > __USE_FILE_OFFSET64. Or more generally: do the same thing as the > non-fortified definition does. >=20 > Hope this makes sense. Yeah, I forgot __fcntl_alias is in fact ... an alias. So in fact there i= s no issue here.