| Message ID | 20230628084246.778302-15-fberat@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allow glibc to be built with _FORTIFY_SOURCE | expand |
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
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 > >
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
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 --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));