Re: [PATCH v3 14/16] libio/bits/stdio2-decl.h: Avoid PLT entries with _FORTIFY_SOURCE

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2023-06-30 11:48, Siddhesh Poyarekar wrote:
> On 2023-06-30 11:38, Frederic Berat wrote:
>>
>>
>> On Fri, Jun 30, 2023 at 5:30 PM Siddhesh Poyarekar 
>> <siddhesh@gotplt.org <mailto:siddhesh@gotplt.org>> wrote:
>>
>>
>>
>>     On 2023-06-28 04:42, Frédéric Bérat wrote:
>>      > The change is meant to avoid unwanted PLT entry for the
>>     fgets_unlocked
>>      > routine when _FORTIFY_SOURCE is set.
>>      > ---
>>      >   libio/bits/stdio2-decl.h | 2 +-
>>      >   1 file changed, 1 insertion(+), 1 deletion(-)
>>      >
>>      > diff --git a/libio/bits/stdio2-decl.h b/libio/bits/stdio2-decl.h
>>      > index 114b06d24b..d7ef7283d6 100644
>>      > --- a/libio/bits/stdio2-decl.h
>>      > +++ b/libio/bits/stdio2-decl.h
>>      > @@ -122,7 +122,7 @@ extern size_t __fread_chk (void *__restrict
>>     __ptr, size_t __ptrlen,
>>      >                          FILE *__restrict __stream) __wur;
>>      >
>>      >   #ifdef __USE_GNU
>>      > -extern char *__REDIRECT (__fgets_unlocked_alias,
>>      > +extern char *__REDIRECT_FORTIFY (__fgets_unlocked_alias,
>>      >                        (char *__restrict __s, int __n,
>>      >                         FILE *__restrict __stream), 
>> fgets_unlocked)
>>      >       __wur __attr_access ((__write_only__, 1, 2));
>>
>>     Why not the same for all the others?
>>
>>
>> I tend to avoid modifying things that are not strictly necessary. If 
>> that happens to be needed on other aliases, then everything is ready 
>> for it, but it seems I didn't stumbled upon a case where it was ...
>> When you look at it, the same way I didn't create 
>> libc_hidden_def/proto for all the routines here (like e.g. 
>> __fread_chk) if that wasn't needed.
> 
> So there's a slight difference; the __REDIRECT_FORTIFY is essentially an 
> assurance that whenever there's an internal use of this function, the 
> alias will redirect to that internal alias.  The hidden_def adds an 
> alias, which is unnecessary until there's an actual internal use and the 
> hidden_proto is only a complement to the hidden_def.
> 
> In that sense, I think the __REDIRECT_FORTIFY should be for all function 
> aliases, not just the ones that are currently being used.

On second thoughts, I think it's OK to leave it as only for functions 
that are actually referenced internally for now.  We can leave it to the 
localplt check to catch the extra PLT refs and then fix this up.

So unless someone else thinks otherwise:

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

Sid