[PATCH] sunrpc: Remove hidden aliases for global data symbols [BZ #26210]

[PATCH] sunrpc: Remove hidden aliases for global data symbols [BZ #26210]

[Florian Weimer]

It is generally not possible to add hidden aliases for global data
symbols: If the main executable contains a copy relocation against
the symbol, the hidden aliases keep pointing to the glibc-internal
copy of the symbol, instead of the symbol actually used by the
application.

Fixes commit 89aacb513eb77549a29df2638913a0f8178cf3f5 ("sunrpc:
Remove stray exports without --enable-obsolete-rpc [BZ #23166]").

Tested on x86_64-linux-gnu, with and without --enable-obsolete-rpc.
Manually checked for compat symbol status.

Okay for glibc 2.32?

---
 include/rpc/clnt.h  |  1 -
 include/rpc/svc.h   |  4 ----
 sunrpc/rpc_common.c | 12 ++++++++----
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/include/rpc/clnt.h b/include/rpc/clnt.h
index 80be0a9cec..a397023a93 100644
--- a/include/rpc/clnt.h
+++ b/include/rpc/clnt.h
@@ -28,7 +28,6 @@ libc_hidden_proto (clntudp_create)
 libc_hidden_proto (get_myaddress)
 libc_hidden_proto (clntunix_create)
 libc_hidden_proto (__libc_clntudp_bufcreate)
-libc_hidden_proto (rpc_createerr)
 
 # endif /* !_ISOMAC */
 #endif
diff --git a/include/rpc/svc.h b/include/rpc/svc.h
index 40ba2546a9..465bf4427d 100644
--- a/include/rpc/svc.h
+++ b/include/rpc/svc.h
@@ -3,10 +3,6 @@
 
 # ifndef _ISOMAC
 
-libc_hidden_proto (svc_pollfd)
-libc_hidden_proto (svc_max_pollfd)
-libc_hidden_proto (svc_fdset)
-
 libc_hidden_proto (xprt_register)
 libc_hidden_proto (xprt_unregister)
 libc_hidden_proto (svc_register)
diff --git a/sunrpc/rpc_common.c b/sunrpc/rpc_common.c
index 2a5d0dc1c7..05abab2a1d 100644
--- a/sunrpc/rpc_common.c
+++ b/sunrpc/rpc_common.c
@@ -48,10 +48,14 @@ libc_hidden_nolink_sunrpc (_null_auth, GLIBC_2_0)
 /* The variables need the nocommon attribute, so that it is possible
    to create aliases and specify symbol versions.  */
 fd_set svc_fdset  __attribute__ ((nocommon));
-libc_hidden_nolink_sunrpc (svc_fdset, GLIBC_2_0)
 struct rpc_createerr rpc_createerr  __attribute__ ((nocommon));
-libc_hidden_nolink_sunrpc (rpc_createerr, GLIBC_2_0)
 struct pollfd *svc_pollfd  __attribute__ ((nocommon));
-libc_hidden_nolink_sunrpc (svc_pollfd, GLIBC_2_2)
 int svc_max_pollfd  __attribute__ ((nocommon));
-libc_hidden_nolink_sunrpc (svc_max_pollfd, GLIBC_2_2)
+#ifdef SHARED
+# ifndef EXPORT_RPC_SYMBOLS
+compat_symbol (libc, svc_fdset, svc_fdset, GLIBC_2_0);
+compat_symbol (libc, rpc_createerr, rpc_createerr, GLIBC_2_0);
+compat_symbol (libc, svc_pollfd, svc_pollfd, GLIBC_2_2);
+compat_symbol (libc, svc_max_pollfd, svc_max_pollfd, GLIBC_2_2);
+# endif
+#endif

Re: [PATCH] sunrpc: Remove hidden aliases for global data symbols [BZ #26210]

[Carlos O'Donell]

On 7/6/20 1:29 PM, Florian Weimer wrote:
> It is generally not possible to add hidden aliases for global data
> symbols: If the main executable contains a copy relocation against
> the symbol, the hidden aliases keep pointing to the glibc-internal
> copy of the symbol, instead of the symbol actually used by the
> application.

Could there have been any way to catch this?

Should we be adding tests to exercise all possible COPY relocations?
 
> Fixes commit 89aacb513eb77549a29df2638913a0f8178cf3f5 ("sunrpc:
> Remove stray exports without --enable-obsolete-rpc [BZ #23166]").
> 
> Tested on x86_64-linux-gnu, with and without --enable-obsolete-rpc.
> Manually checked for compat symbol status.
> 
> Okay for glibc 2.32?

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> ---
>  include/rpc/clnt.h  |  1 -
>  include/rpc/svc.h   |  4 ----
>  sunrpc/rpc_common.c | 12 ++++++++----
>  3 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/include/rpc/clnt.h b/include/rpc/clnt.h
> index 80be0a9cec..a397023a93 100644
> --- a/include/rpc/clnt.h
> +++ b/include/rpc/clnt.h
> @@ -28,7 +28,6 @@ libc_hidden_proto (clntudp_create)
>  libc_hidden_proto (get_myaddress)
>  libc_hidden_proto (clntunix_create)
>  libc_hidden_proto (__libc_clntudp_bufcreate)
> -libc_hidden_proto (rpc_createerr)

OK.

>  
>  # endif /* !_ISOMAC */
>  #endif
> diff --git a/include/rpc/svc.h b/include/rpc/svc.h
> index 40ba2546a9..465bf4427d 100644
> --- a/include/rpc/svc.h
> +++ b/include/rpc/svc.h
> @@ -3,10 +3,6 @@
>  
>  # ifndef _ISOMAC
>  
> -libc_hidden_proto (svc_pollfd)
> -libc_hidden_proto (svc_max_pollfd)
> -libc_hidden_proto (svc_fdset)

OK. I was specifically thinking of svc_fdset when I read the above
description.

> -
>  libc_hidden_proto (xprt_register)
>  libc_hidden_proto (xprt_unregister)
>  libc_hidden_proto (svc_register)
> diff --git a/sunrpc/rpc_common.c b/sunrpc/rpc_common.c
> index 2a5d0dc1c7..05abab2a1d 100644
> --- a/sunrpc/rpc_common.c
> +++ b/sunrpc/rpc_common.c
> @@ -48,10 +48,14 @@ libc_hidden_nolink_sunrpc (_null_auth, GLIBC_2_0)
>  /* The variables need the nocommon attribute, so that it is possible
>     to create aliases and specify symbol versions.  */
>  fd_set svc_fdset  __attribute__ ((nocommon));
> -libc_hidden_nolink_sunrpc (svc_fdset, GLIBC_2_0)
>  struct rpc_createerr rpc_createerr  __attribute__ ((nocommon));
> -libc_hidden_nolink_sunrpc (rpc_createerr, GLIBC_2_0)
>  struct pollfd *svc_pollfd  __attribute__ ((nocommon));
> -libc_hidden_nolink_sunrpc (svc_pollfd, GLIBC_2_2)
>  int svc_max_pollfd  __attribute__ ((nocommon));
> -libc_hidden_nolink_sunrpc (svc_max_pollfd, GLIBC_2_2)
> +#ifdef SHARED
> +# ifndef EXPORT_RPC_SYMBOLS
> +compat_symbol (libc, svc_fdset, svc_fdset, GLIBC_2_0);
> +compat_symbol (libc, rpc_createerr, rpc_createerr, GLIBC_2_0);
> +compat_symbol (libc, svc_pollfd, svc_pollfd, GLIBC_2_2);
> +compat_symbol (libc, svc_max_pollfd, svc_max_pollfd, GLIBC_2_2);

OK.

> +# endif
> +#endif
> 


-- 
Cheers,
Carlos.

Re: [PATCH] sunrpc: Remove hidden aliases for global data symbols [BZ #26210]

[Florian Weimer]

* Carlos O'Donell:

> On 7/6/20 1:29 PM, Florian Weimer wrote:
>> It is generally not possible to add hidden aliases for global data
>> symbols: If the main executable contains a copy relocation against
>> the symbol, the hidden aliases keep pointing to the glibc-internal
>> copy of the symbol, instead of the symbol actually used by the
>> application.
>
> Could there have been any way to catch this?

Code review?

> Should we be adding tests to exercise all possible COPY relocations?

Or watch out for global data symbols without relocations against them.
But it's quite an obvious bug in retrospect.  _null_auth in the same
file probably confused me—it's suspicious as well, but changing it after
eleven years does not make much sense to me.

>> Fixes commit 89aacb513eb77549a29df2638913a0f8178cf3f5 ("sunrpc:
>> Remove stray exports without --enable-obsolete-rpc [BZ #23166]").
>> 
>> Tested on x86_64-linux-gnu, with and without --enable-obsolete-rpc.
>> Manually checked for compat symbol status.
>> 
>> Okay for glibc 2.32?
>
> OK for master.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks.

Florian

Re: [PATCH] sunrpc: Remove hidden aliases for global data symbols [BZ #26210]

[Carlos O'Donell]

On 7/7/20 4:04 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 7/6/20 1:29 PM, Florian Weimer wrote:
>>> It is generally not possible to add hidden aliases for global data
>>> symbols: If the main executable contains a copy relocation against
>>> the symbol, the hidden aliases keep pointing to the glibc-internal
>>> copy of the symbol, instead of the symbol actually used by the
>>> application.
>>
>> Could there have been any way to catch this?
> 
> Code review?

I'm thinking more along the lines of a regression test that has a list
of things to check in this area and fails when you change that list
and then have to update it by hand which forces you to think about
this particular issue.

Similar to how we have local plt lists and tests.

>> Should we be adding tests to exercise all possible COPY relocations?
> 
> Or watch out for global data symbols without relocations against them.

Yes.

> But it's quite an obvious bug in retrospect.  _null_auth in the same
> file probably confused me—it's suspicious as well, but changing it after
> eleven years does not make much sense to me.

Sure.

>>> Fixes commit 89aacb513eb77549a29df2638913a0f8178cf3f5 ("sunrpc:
>>> Remove stray exports without --enable-obsolete-rpc [BZ #23166]").
>>>
>>> Tested on x86_64-linux-gnu, with and without --enable-obsolete-rpc.
>>> Manually checked for compat symbol status.
>>>
>>> Okay for glibc 2.32?
>>
>> OK for master.
>>
>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
> Thanks.
> 
> Florian
> 


-- 
Cheers,
Carlos.