diff mbox series

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

Message ID 20230628084246.778302-15-fberat@redhat.com
State Superseded
Headers
Series Allow glibc to be built with _FORTIFY_SOURCE |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 pending Test started
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 pending Test started
linaro-tcwg-bot/tcwg_glibc_check--master-arm pending Test started
linaro-tcwg-bot/tcwg_glibc_build--master-arm pending Test started

Commit Message

Frederic Berat June 28, 2023, 8:42 a.m. UTC 
  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(-)

Comments

Siddhesh Poyarekar June 30, 2023, 3:30 p.m. UTC | #1 
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?

Sid
Frederic Berat June 30, 2023, 3:38 p.m. UTC | #2 
On Fri, Jun 30, 2023 at 5:30 PM Siddhesh Poyarekar <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.


>
> Sid
>
>
Siddhesh Poyarekar June 30, 2023, 3:48 p.m. UTC | #3 
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.

Sid
Siddhesh Poyarekar June 30, 2023, 5:08 p.m. UTC | #4 
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
diff mbox series

Patch

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));