From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com [IPv6:2607:f8b0:4864:20::22f]) by sourceware.org (Postfix) with ESMTPS id 1AD7F3858430 for ; Tue, 23 May 2023 19:43:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1AD7F3858430 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-x22f.google.com with SMTP id 5614622812f47-38eda4ef362so126539b6e.2 for ; Tue, 23 May 2023 12:43:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684871025; x=1687463025; 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=HFkRrihjVAaYGQqkkZtS2kC8R3V6sunWTpdln1TTHQM=; b=oauVJ4qldfnv+VMllH1nvz8WkGA7xS49eP8yKWKavMWpP5sW1RoSBvsBvPrala4jlV 5zNRxjPOyzbKUMC6266NQG9lpUCnP1GOf/e51G6i77LV1aks4IvW0O5HjknBizxZg3Dh RmMbppdIQ53omoJ9obs9BWgmeTj39jZqhclfM+3dSoBVi9C99NXtZfSXC9EuGtO5F1Mb +o5bCqlaB0nBih50RRrvAShEIlRSIzikgL7SjNI9skeBqi4E4rnxNVlpkM5tq2b25o9z D8/pkUU54Oqh9Rf4PwXv5WUB4IA8h0l7/bsBubkdEkUkuJBNlDpm4owPPc9g6hTI7+Ps pTWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684871025; x=1687463025; 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=HFkRrihjVAaYGQqkkZtS2kC8R3V6sunWTpdln1TTHQM=; b=NwO5i9YPd9NZWYz75fldIXSFPB2x1QZBEKT1zBnwv8C6p+NjB/T3X58C+xTKAThQV0 PlYRonfjmCQgvuieb49fFs0VJnfMbs44UXlapHjxuc71bTgZzAirwe6nyQsGVvvv3573 USx3ritxUw1kr0AVN/5Qjjn4rorHY7tSc/kqWAEkyMiyBNDsuZNEFUu8aLJiMCY8Hozm +LgOXRlzIJCX208mLWT9x9YVwryPuN5um48gyTqhn6ZrW6rjbxo3zreJnQ9/wRRa3Xsp olPoam45eTozQ7CqdHGn6LPxKwb7oMZ4ZWmsD9wqSb8TZt7GsaZ9qBMoLVJUNL8ForOf HEBA== X-Gm-Message-State: AC+VfDyxqfegoGKw95hWKUbCO+pJfTHzdR39pYOJyOvmAjnBfBCHhAZN c+3Ni9bcMivq1TgH+Dy7LS2wvtdgHfVo7xic/2HsqiKqZ28= X-Google-Smtp-Source: ACHHUZ7fmfGDSI1II/As31gtja3TbTxXHaCyhRVHA5FtTl/Bc3h1NV4QJMB24vzVOfiRLveIlfgln19HDc5APxq1q3k= X-Received: by 2002:a05:6808:a83:b0:396:1f76:b5be with SMTP id q3-20020a0568080a8300b003961f76b5bemr8435930oij.14.1684871025161; Tue, 23 May 2023 12:43:45 -0700 (PDT) MIME-Version: 1.0 References: <20230519213059.3812385-1-bugaevc@gmail.com> <20230519213059.3812385-2-bugaevc@gmail.com> <0b512019-09ab-9f25-57d6-cf6fd19be8f3@linaro.org> In-Reply-To: <0b512019-09ab-9f25-57d6-cf6fd19be8f3@linaro.org> From: Sergey Bugaev Date: Tue, 23 May 2023 22:43:34 +0300 Message-ID: Subject: Re: [RFC PATCH 1/1] 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.6 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 Tue, May 23, 2023 at 10:09=E2=80=AFPM Adhemerval Zanella Netto wrote: > > +# ifndef __USE_FILE_OFFSET64 > > +/* Just fcntl (). */ > > Does this comment adds anything? Also I think we avoid to use '()' on fu= nction > definition on comments. These comments were mainly for myself to keep track of the various possible configurations -- I decided to leave them in since the reader of the code might get a little overwhelmed by the combinatorial explosion of the possible configuration variants as well. I can remove these comments (or just this one?) if you would prefer that. > > +__extern_always_inline int > > Should we use __fortify_function also for this case? I think __fortify_function is supposed to be used on the replacement functions themselves, not their helpers? if that's not the case, sure. > > +#ifdef __F_GETOWN_EX > > + case __F_GETOWN_EX: > > + case __F_SETOWN_EX: > > + case __F_SETSIG: > > +#endif > > I think it would be better to condicionalize using __USE_GNU: > > #ifdef __USE_GNU > case F_GETOWN_EX: > case F_SETOWN_EX: > case F_SETSIG: > #endif > > Since the double underscore symbols are implementation defined one and sh= ould > not be used by the application. But that would break on the Hurd, no? I could do #ifdef F_GETOWN_EX (without the double underscore), but I wanted to handle __F_GETOWN_EX etc. even if the user has not enabled them (better handle them than not). In other words, this ifdef is meant to check for Hurd vs Linux, not for __USE_GNU vs not. > Should be worried that since we don't have const expressions like newer C= ++ > that large switch might ended being extra code on every fcntl call? I thi= nk > the __builtin_constant_p check below gives compiler enough information to > avoid it, but I am not sure. Yes, __builtin_constant_p is, AFAIK, some compiler magic that "guarantees" (quoted since I'm not sure if it's actually guaranteed, but I've never seen it behave otherwise) that in the true branch the value is treated as a compile-time constant. In practice (see what the tst-fortify compiles to!), with -O2 this whole thing is fully optimized away, and the generated code contains the very same calls to fcntl (or fcntl64, or whatever) that it would if there never was any fortification -- except you get compile-time errors if you forget the argument when it's required. So there's zero run-time cost. If the cmd argument is a c-t const, that is -- otherwise it calls the _2 versions and does the check at runtime, yes. But that's only extra code (as in code size) once, in glibc, not at the call sites. > > + if (__fcntl_cmd_needs_arg (__cmd) && __va_arg_pack_len () < 1) > > No implicit check for function that do not return bool: > > if (__fcntl_cmd_needs_arg (__cmd) =3D=3D 1 ...) Why? Is it just a code style thing? > I think we need also to handle __USE_TIME_BITS64 and add another fcntl de= bug > symbol to handle; even though for Linux now __fcntl_time64 is just a alia= s > to __libc_fcntl64. > > It is because we even need to add a proper __fcntl_time64 implementation, > the redirection now does: > > fcntl -> __fcntl64_2 -> __libc_fcntl64 > > Where it should be something like: > > fcntl -> __fcntl_time64_2 -> __fcntl_time64 The idea here was that the 2-argument version of fcntl clearly does not deal with time, either 32- or 64-bit... Sergey