Re: [PATCH v2 3/3] io: Add FORTIFY_SOURCE check for fcntl arguments

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 29/05/23 18:59, Sergey Bugaev wrote:
> On Tue, May 30, 2023 at 12:09 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>> It is returned unmodified, but the asm acts a compiler barrier which gcc
>> documentation also declares as 'strong memory barrier' [1] (which I think was
>> written before C11 memory semantic).
>>
>> [1] https://gcc.gnu.org/onlinedocs/gcc/Volatiles.html
> 
> But that one is talking about load/store reordering, not constant
> propagation. Here's what I'm talking about: [0] (and an actual Rust
> version at [1]).
> 
> [0]: https://godbolt.org/z/qaMa7EavY
> [1]: https://godbolt.org/z/c4dhaKbqe
> 
> Would you like me to add something similar to Rust's black_box
> glibc-wide in a header (if so, what would be a good name?), or should
> I just do it locally in tst-fortify.c where I need it?

Ah, I see it now.  I am not sure if it would change the benchmarks we have,
since the idea is only to prevent compiler optimize away function calls
that might have no side-effects.  But it does seems a useful addition for
internal header, maybe tune the name a bit.

> 
>>> Rust's black_box is / was [0] instead implemented as
>>>
>>> llvm_asm!("" : : "r"(&mut dummy) : "memory" : "volatile");
>>
>> What the 'volatile' constraint does for the llvm_asm? Is is to mimic a
>> 'asm volatile' or is something else?
> 
> Yes, I believe it was the same thing as 'asm volatile'. This is
> because llvm_asm!() was not a special *syntax* like inline asm is in
> GCC, but a magic macro, so 'volatile' has to go inside the macro.
> 
> But also note that llvm_asm!() has been deprecated and removed [2] (it
> was always an unstable feature, never intended to be stabilized); it's
> been replaced by the new asm!() macro that has a different mini-syntax
> (more like the Rust formatting macros, less like GCC/LLVM inline
> assembly).
> 
> [2]: https://github.com/rust-lang/rust/pull/92816
> 
> With the asm!() macro the same would rather look like this:
> 
> asm!(
>     "/* pretend to use {0} */",
>     in(reg) &mut dummy,
>     options(nostack, preserves_flags)
> )
> 
> The default around 'volatile' (and other flags) has been flipped, now
> you'd need to specify 'options(pure)' to get the previous non-volatile
> behavior.

Thanks for the explanation.

> 
>> It is exported because all tests are actually built with _GNU_SOURCE (done
>> by include/libc-symbols.h), so the test check is superfluous.  It also
>> leaks implementation details, such as internal defines.  Usually to check
>> for internal implementation we use test-internal (which are built
>> statically).
>>
>> But if you really want to check for _LARGEFILE64_SOURCE, you will need to
>> add *another* fortify test that undef _GNU_SOURCE (like stdlib/tst-strtol-binary-c11.c
>> for instance).
> 
> I thought the
> 
> src-chk-nongnu = \#undef _GNU_SOURCE
> 
> part handled undoing the #define _GNU_SOURCE (from
> include/libc-symbols.h) for the -nongnu- tests?

But at same time we always use -D_LARGEFILE64_SOURCE=1 for nognu:

  CFLAGS-tst-fortify-$(1)-nongnu-$(2)-$(3).$(1) += -D_LARGEFILE64_SOURCE=1

We have some overlap on current way to organize tst-fortify, but I think
it should cover everything:

  1. tst-fortify-c-default: uses _GNU_SOURCE, provides both LFS and non-LFS,
     no redirections.
  2. tst-fortify-c-lfs: uses _GNU_SOURCE and _FILE_OFFSET_BITS=64, provides
     both LFS and non-LFS, redirects non-LFS calls to LFS.
  3. tst-fortify-c-nongnu: undef _GNU_SOURCE, defines _LARGEFILE64_SOURCE,
     provides both LFS and non-LFS, no redirections.

I guess you might add another configuration to undef _GNU_SOURCE and 
define _LARGEFILE64_SOURCE; but I don't think it would increase coverage.

> 
> I also think I might have actually gotten errors here about missing
> fcntl64 and the like before I added the ifdef check, but I might be
> misremembering this one.
> 
> Sergey