Re: [PATCH v3 0/5] fcntl fortification

[Carlos O'Donell <carlos@redhat.com>]

On 6/17/23 18:22, Sergey Bugaev via Libc-alpha wrote:
> Hello,
> 
> this is v3 of the fcntl fortification work. v1 was at [0], v2 at [1].
> 
> [0]: https://sourceware.org/pipermail/libc-alpha/2023-May/148332.html
> [1]: https://sourceware.org/pipermail/libc-alpha/2023-May/148569.html

Pre-commit CI regression caused by this series on both aarch64 and aarch32.
https://patchwork.sourceware.org/project/glibc/patch/20230617222218.642125-6-bugaevc@gmail.com/

# Running glibc:io ...
# FAIL: io/check-installed-headers-c 
# FAIL: io/check-installed-headers-cxx 
# 
# Running glibc:misc ...
# FAIL: misc/check-installed-headers-c 
# FAIL: misc/check-installed-headers-cxx 
# 
# Running glibc:rt ...
# FAIL: rt/check-installed-headers-c 
# FAIL: rt/check-installed-headers-cxx 


> Changes since v2:
> 
> - This is now in its own separate header, bits/fcntl3.h
>   (bits/fcntl2.h is used by the open fortification)
> 
> - Clang is now supported in addition to GCC!
>   - Clang does not support nor need the "-Wsystem-headers" pragma
>   - Clang does support error/warning attributes since recently
>   - There seems to be a bug in Clang which prevents the type mismatch
>     warning from actually firing. Specifically, it appears that Clang
>     gets confused about C functions names vs symbol names when it comes
>     to attribute ((warning)), and does not emit the warning if the
>     function declared with __warnattr has a symbol name matching that of
>     another function that has not been declared with __warnattr.
> 
>     While this could be worked around in glibc (such as by adding
>     __fcntl_warn as a real wrapper function when built with Clang), I
>     think this just needs to be fixed in Clang. Any LLVM developers
>     here? :D
> 
> - Changed hide_constant utility to use an empty inline asm statement
>   instead of volatile and noinline, as per the discussion. I did not
>   make this into a general-purpose glibc-wide utility because I don't
>   know what a fitting name and place (header) for it would be. If you'd
>   like to see it glibc-wide, please suggest me where to put it and how
>   to name it!
> 
> - Fixed the C++ template linkage thing
> 
> - Addressed misc review comments
> 
> - Looked into applying __builtin_constant_p to the result of the cmd
>   check and not the cmd value itself, as suggested by Florian.
> 
>   Unfortunately this does not work at all :( __builtin_constant_p starts
>   returning 0 given anything remotely complex like even a trivial inline
>   function call (so technically hide_constant would still work if it was
>   just 'return value;'), even if the function is (later?) fully inlined
>   and const-folded.
> 
>   *Maybe* this could be made to work if I used an obscene amount of
>   macros instead of inline functions (to handle all the various commands
>   being conditionally defined), but I don't want to go there.
> 
>   So since this didn't work out, I left the runtime __fcntl_2 function,
>   but split it into a separate patch, so you can apply it or drop it
>   depending on what you prefer in the end.
> 
> Clang / C++ demo:
> 
> ------------------------------------------------------------------
> $ clang++ test-fcntl.cpp -D _FORTIFY_SOURCE=2 -O2
> In file included from test-fcntl.cpp:1:
> In file included from /usr/include/fcntl.h:348:
> /usr/include/bits/fcntl3.h:394:5: error: call to '__fcntl_missing_arg' declared with 'error' attribute: fcntl with with this command needs 3 arguments
>     __fcntl_missing_arg ();
>     ^
> 1 error generated.
> ------------------------------------------------------------------
> 
> Sergey
> 

-- 
Cheers,
Carlos.