From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oo1-xc2f.google.com (mail-oo1-xc2f.google.com [IPv6:2607:f8b0:4864:20::c2f]) by sourceware.org (Postfix) with ESMTPS id 045C7385840C for ; Mon, 29 May 2023 19:57:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 045C7385840C 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-oo1-xc2f.google.com with SMTP id 006d021491bc7-5576c2791e6so2161261eaf.1 for ; Mon, 29 May 2023 12:57:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685390233; x=1687982233; 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=PQlHmlZ00+ayfyDrs618EPuY6sMgveAKWIJtv1uxXdc=; b=eqRW0eLU9GwRa7tnF2VcqXiF3lm+i10zWHM1frYo9SzMmwPVtAPnLDkPX+kdg++wGP 6PEqcPqH7UG5Ao86veiZBqu5IUtWfhGVd6R55NmnYJYLHDvBxAL0yhzFNEw6AH2rKUjM 0m4JOqSxeMiqTJKVpIaZLueV75xptQ0PzD7xVibA2lHD20uhjudo3zodaiI7jrATG/Jp RYmYda34FG3PI4iCMsDWmXcZrKj/Bo2+v/Hy+FA+ET66N7+RkfGAXIZE6jwkqq2VQ/HC eos56ZgRYEeMFlQaYOdCetsyH1qZlen3Sq8NuMr0QPi9vFkiqeaPZwygkzEfhBFj7f4y M3ug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685390233; x=1687982233; 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=PQlHmlZ00+ayfyDrs618EPuY6sMgveAKWIJtv1uxXdc=; b=OHV3eOfzkpp+eRP712zRVNJbg+E5myaRufcRkqcE6lFXbjLo1VVvb3XOw1T553c9Rm 0nFOO43K/W0p2eaCIRSWMnlCP7oV7kILPBg2she7XzJ8JdRDO3vCocvY+g9FMd6CAsps CW+Zheh+PnIwU92giDOjHNWfQIONe0syIoXzmKrBWEpEb62gc8G+IQ3IUKTv7/F1m2LM pRO1rqzbNnQHLqw9zMxUcG9gluDGrjBZwA3UkKzJWCJrC/OHx6S0wOMg8mwBQWX/S5PI uU/yCpP11F13ET2pjNeXx0upFJnhDkjMKzBJ3Q1g8Ua1p0vq9lpEf4XmfXo5a9JZbE09 Oy6Q== X-Gm-Message-State: AC+VfDzxxuIr/PUH7CVeIu5UE+I2qhnOKfWRgpfXi81Q1KgEkFaN09Ez egyN/Y/DAhJ8tP86RxfkGydYFk4I0L3X9bQgz8c= X-Google-Smtp-Source: ACHHUZ6PU4azvyJ1j1t6pwmTeC0qCzmdpl6E1NiSDSF6de4q2tZ5vQHQc07APij1C68uPXLX7ZFHRUuilyAUnQAXpHw= X-Received: by 2002:a05:6808:1823:b0:399:169:75d4 with SMTP id bh35-20020a056808182300b00399016975d4mr49824oib.36.1685390233125; Mon, 29 May 2023 12:57:13 -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> <8354c659-cfb0-993a-2764-72a2cd6f6ed4@linaro.org> In-Reply-To: <8354c659-cfb0-993a-2764-72a2cd6f6ed4@linaro.org> From: Sergey Bugaev Date: Mon, 29 May 2023 22:57:02 +0300 Message-ID: Subject: Re: [PATCH v2 3/3] io: Add FORTIFY_SOURCE check for fcntl arguments To: Adhemerval Zanella Netto Cc: Florian Weimer , libc-alpha@sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.2 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: 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 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 > > It is because afaiu 'volatile' does not really match the semantics of wha= t you > are trying to do here. Maybe if you really need the compile to force lva= lue > evaluation, it should use something similar to math_opt_barrier. 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. > Nevermind, you do need to check for __USE_LARGEFILE64 symbols. But > you can omit the '#if defined", they are always defined. 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. > 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 ca= ll is: > > res =3D (0 ? __fcntl_2_inline (0, 2) : !__builtin_constant_p (2) ? __fcnt= l_alias (0, 2, 0) : (__fcntl_is_int (2) ? __builtin_types_compatible_p (__t= ypeof (0), int) : __fcntl_is_const_uint64_t_ptr (2) ? (__builtin_types_comp= atible_p (__typeof (0), const __uint64_t *) || __builtin_types_compatible_p= (__typeof (0), __uint64_t *)) : __fcntl_is_uint64_t_ptr (2) ? __builtin_ty= pes_compatible_p (__typeof (0), __uint64_t *) : __fcntl_is_const_fowner_ex = (2) ? (__builtin_types_compatible_p (__typeof (0), const struct f_owner_ex = *) || __builtin_types_compatible_p (__typeof (0), struct f_owner_ex *)) : _= _fcntl_is_fowner_ex (2) ? __builtin_types_compatible_p (__typeof (0), struc= t f_owner_ex *) : __fcntl_is_const_flock (2, 0) ? (__builtin_types_compatib= le_p (__typeof (0), const struct flock *) || __builtin_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_compatible_p (__typeof (0), struct floc= k64 *) : 1) ? __fcntl_alias (0, 2, 0) : __fcntl_warn (0, 2, 0)); > > Which is the non-LFS version. 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.) 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. > 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 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. Hope this makes sense. Sergey