From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22d.google.com (mail-oi1-x22d.google.com [IPv6:2607:f8b0:4864:20::22d]) by sourceware.org (Postfix) with ESMTPS id 8C2B53858D32 for ; Mon, 29 May 2023 18:09:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8C2B53858D32 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-oi1-x22d.google.com with SMTP id 5614622812f47-3982f09df74so2011903b6e.0 for ; Mon, 29 May 2023 11:09:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1685383783; x=1687975783; 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=ZkYsOAXeAMc+91GcVjbSTsmef61guIbTCTVGZ/w0pKM=; b=Bmdvh2Z40LLc1bc5tIe+xQ6+D6CYY4ibAOfcPM7jhlSmhvRhihjI3LeDOzWQLQ9QKC 9k2IU8E6WAZdlvUIoNs2T03PUDHjgyriTrtS1u1R1SFvW1E3q6GQtIOz9/cA+7ED0RNe hByPSLzgA4GchErWZuZEFzY5QABNECUqeVMCAcCFQNcw7ycTc1792+80QgNnQpHW0jX/ atpXXobgtJgoM4vlBT2AtlKiChBHMWllU+mzx+0nLMJMZpZdgXt61shnC1ipaKp2+ZwV CuUo0jCrrpErXKWkk8gmhNBC/cBCv1rnMGgmqxUxsK81dNai/OeshseswN6iWQQupQs8 d1NA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685383783; x=1687975783; 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=ZkYsOAXeAMc+91GcVjbSTsmef61guIbTCTVGZ/w0pKM=; b=AXEpmLxI9y0ZuYa8ZIWCnMbpjWWwEwimcWB8tLXqDZWFkPShYzaQmjma8nuOU3SirM 3f13OvEMlk7lgW24DceSy+Qax3nELo3LaKqrjDsFhGKibjg555KGGf5BRB6xnds1LeVU mvPHBqAfzcr5MGhoeOQwJVdd6TiQsobO8+NxrwSUtIorT/2+6XwqFiF6Y1AmtuYliTyl 5V+LAFfqBh9ndN98QtjQOc7XQXopWsCMIL81HOwv/HFgui7f8alw8VVI9tbaIMKoeqPR uqDjht3GzUeqkyCD19DcvxFeN2p9wXIjQlQirIiktMnV9bjgdRs81gBxRn9YzZ4TA1bM uptg== X-Gm-Message-State: AC+VfDz4aag1nwrDRbE7cWEYEuuw/5op5hGwIClJ/XV2+oaOA4mbiYkC 8PmX1gOoYE7R5P4mIJ1rbupFHw== X-Google-Smtp-Source: ACHHUZ4q8hiUGHgyyc0XWtBSun2bCxnerzJvcQ+KXNs6kdVDL9nkMdYo3wgCRiuPbX9vRQQccwyRww== X-Received: by 2002:a05:6808:1390:b0:397:f15e:fc8 with SMTP id c16-20020a056808139000b00397f15e0fc8mr6840760oiw.35.1685383782796; Mon, 29 May 2023 11:09:42 -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 h2-20020a4ae8c2000000b0054fd0b7af2bsm4403588ooe.31.2023.05.29.11.09.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 29 May 2023 11:09:41 -0700 (PDT) Message-ID: <8354c659-cfb0-993a-2764-72a2cd6f6ed4@linaro.org> Date: Mon, 29 May 2023 15:09:39 -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 , Florian Weimer Cc: libc-alpha@sourceware.org References: <20230528172013.73111-1-bugaevc@gmail.com> <20230528172013.73111-4-bugaevc@gmail.com> <31457dbb-a805-262f-4b62-be0b40960ca6@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.3 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 14:31, Sergey Bugaev wrote: > Hello, >=20 > On Mon, May 29, 2023 at 7:54=E2=80=AFPM Adhemerval Zanella Netto > wrote: >> Some comments below. >=20 > thank you for the review :) >=20 >>> +/* 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 h= ere. >> Afaiu what really prevents compiler to return fortify compiler warning= here >> is the __attribute_noinline__. >=20 > 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.>=20 > [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. >=20 >>> + struct flock flock; >> >> Maybe move the flock struct to within the block for where ofd_locks_su= pported >> is true? >=20 > It's used in the following if (ofd_locks_supported) block -- the one > checking for non-compile-time-const cmd -- too. Yes, but the idea is to avoid the stack allocated variable to escape its specific usage (Florian is the one that constantly nag about it). >=20 >>> +#if defined (__USE_LARGEFILE64) || defined (__USE_TIME_BITS64) >> >> There is no need to replicate the tests for the LFS support, this is a= lready >> done by tst-fortify-cc-lfs-X tests. >=20 > Hm, I don't think I understand your point, could you please expand? Nevermind, you do need to check for __USE_LARGEFILE64 symbols. But you can omit the '#if defined", they are always defined. >=20 > 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. And debugging a bit, it seems that I found another issue. Building=20 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 (_= _typeof (0), int) : __fcntl_is_const_uint64_t_ptr (2) ? (__builtin_types_= compatible_p (__typeof (0), const __uint64_t *) || __builtin_types_compat= ible_p (__typeof (0), __uint64_t *)) : __fcntl_is_uint64_t_ptr (2) ? __bu= iltin_types_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_ow= ner_ex *)) : __fcntl_is_fowner_ex (2) ? __builtin_types_compatible_p (__t= ypeof (0), struct f_owner_ex *) : __fcntl_is_const_flock (2, 0) ? (__buil= tin_types_compatible_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 *) : __fcn= tl_is_const_flock64 (2, 0) ? (__builtin_types_compatible_p (__typeof (0),= const struct flock64 *) || __builtin_types_compatible_p (__typeof (0), s= truct flock64 *)) : __fcntl_is_flock64 (2, 0) ? __builtin_types_compatibl= e_p (__typeof (0), struct flock64 *) : 1) ? __fcntl_alias (0, 2, 0) : __f= cntl_warn (0, 2, 0)); Which is the non-LFS version. For non macro calls, pread for instance, we fix this by using the __glibc_fortify version: # ifndef __USE_FILE_OFFSET64 __fortify_function __wur ssize_t pread (int __fd, void *__buf, size_t __nbytes, __off_t __offset) { return __glibc_fortify (pread, __nbytes, sizeof (char), __glibc_objsize0 (__buf), __fd, __buf, __nbytes, __offset); } # else __fortify_function __wur ssize_t pread (int __fd, void *__buf, size_t __nbytes, __off64_t __offset) { return __glibc_fortify (pread64, __nbytes, sizeof (char), __glibc_objsize0 (__buf), __fd, __buf, __nbytes, __offset); } # endif 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 #ifdef __USE_LARGEFILE64 # define fcntl64(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__, 1) = \ ? __fcntl64_alias (fd, cmd, __VA_ARGS__) = \ : __fcntl64_warn (fd, cmd, __VA_ARGS__))) #endif >=20 > __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? >=20 >>> +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 l= inkage >> 460 | template >> | ^~~~~~~~ >> ../misc/sys/cdefs.h:140:25: note: =E2=80=98extern "C"=E2=80=99 linkage= started here >> 140 | # define __BEGIN_DECLS extern "C" { >=20 > Interesting. Any idea why this wasn't failing in my testing? My guess is because template with extern "C" are not really supported in = all configurations. >=20 >> You will will need to include bits/fcntl2.h after the __END_DECLS, and= add >> __BEGIN_DECLS/__END_DECLS on fcntl2.h. Something like this on top thi= s >> patch >=20 > Right, makes sense, thank you. >=20 >> I think we will need to enable fcntl fortify only for gcc 8 or higher,= since >> __VA_OPT__ is not support on gcc 7. >=20 > 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. Yes, it would be better indeed.