Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments

[Sergey Bugaev <bugaevc@gmail.com>]

On Tue, May 23, 2023 at 10:09 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> > +# ifndef __USE_FILE_OFFSET64
> > +/* Just fcntl ().  */
>
> Does this comment adds anything?  Also I think we avoid to use '()' on function
> 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 should
> 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 think
> 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) == 1 ...)

Why? Is it just a code style thing?

> I think we need also to handle __USE_TIME_BITS64 and add another fcntl debug
> symbol to handle; even though for Linux now __fcntl_time64 is just a alias
> 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