diff mbox series

sunrpc: Turn bindresvport into a compatibility symbol

Message ID 87k0zfwbjj.fsf@oldenburg2.str.redhat.com
State Superseded
Headers show
Series sunrpc: Turn bindresvport into a compatibility symbol | expand

Commit Message

Florian Weimer July 7, 2020, 4:10 p.m. UTC
(This is a bit of a controversial change, but I'm not sure what our
alternatives are, given the conflict with libtirpc.)

bindresvport is mostly used along with Sun RPC functions.  libtirpc
has its own implementation of it, and this implementation is
is used when linking against libtirpc (under a separate symbol
version).  The libtirpc implementation is superior because it
provides a filter for well-known ports, addressing historic bug 456.

Exceptions that use bindresvport without linking against libtirpc
are torque (the resource manager) and the sign command in OBS
(the build service).

This change turns bindresvport into a compatibility symbol because
it is currently declared in a non-RPC header (<netinet/in.h>),
and remvoes it from the public header.  In theory, it would be
possible to make the declaration conditional on
--enable-obsolete-rpc, but that would need quite a bit of shuffling
for a declaration that would be removed soon anyway.

Despite the declaration of bindresvport6 in a public header file,
no implemention was provided by glibc, so this commit removes this
declaration without further changes.

---
 NEWS                 |  3 +++
 include/netinet/in.h |  4 +++-
 inet/netinet/in.h    | 10 ----------
 sunrpc/bindrsvprt.c  |  9 +++++++--
 sunrpc/clnt_tcp.c    |  2 +-
 sunrpc/clnt_udp.c    |  2 +-
 sunrpc/svc_tcp.c     |  2 +-
 sunrpc/svc_udp.c     |  2 +-
 8 files changed, 17 insertions(+), 17 deletions(-)

Comments

H.J. Lu July 7, 2020, 4:34 p.m. UTC | #1
On Tue, Jul 7, 2020 at 9:13 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> (This is a bit of a controversial change, but I'm not sure what our
> alternatives are, given the conflict with libtirpc.)
>
> bindresvport is mostly used along with Sun RPC functions.  libtirpc
> has its own implementation of it, and this implementation is
> is used when linking against libtirpc (under a separate symbol
> version).  The libtirpc implementation is superior because it
> provides a filter for well-known ports, addressing historic bug 456.
>
> Exceptions that use bindresvport without linking against libtirpc
> are torque (the resource manager) and the sign command in OBS
> (the build service).

Have they been fixed?  Otherwise, they won't compile/link with glibc
2.32.

> This change turns bindresvport into a compatibility symbol because
> it is currently declared in a non-RPC header (<netinet/in.h>),
> and remvoes it from the public header.  In theory, it would be
> possible to make the declaration conditional on
> --enable-obsolete-rpc, but that would need quite a bit of shuffling
> for a declaration that would be removed soon anyway.
>
> Despite the declaration of bindresvport6 in a public header file,
> no implemention was provided by glibc, so this commit removes this
> declaration without further changes.

I think that bindresvport6 can be removed with a separate patch.
Florian Weimer July 7, 2020, 5:04 p.m. UTC | #2
* H. J. Lu:

> On Tue, Jul 7, 2020 at 9:13 AM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>> (This is a bit of a controversial change, but I'm not sure what our
>> alternatives are, given the conflict with libtirpc.)
>>
>> bindresvport is mostly used along with Sun RPC functions.  libtirpc
>> has its own implementation of it, and this implementation is
>> is used when linking against libtirpc (under a separate symbol
>> version).  The libtirpc implementation is superior because it
>> provides a filter for well-known ports, addressing historic bug 456.
>>
>> Exceptions that use bindresvport without linking against libtirpc
>> are torque (the resource manager) and the sign command in OBS
>> (the build service).
>
> Have they been fixed?  Otherwise, they won't compile/link with glibc
> 2.32.

I suppose I can try to submit patches for them if this indeed the
direction we want to move in.

The symbol clash with libtirpc is rather unfortunate.

>> This change turns bindresvport into a compatibility symbol because
>> it is currently declared in a non-RPC header (<netinet/in.h>),
>> and remvoes it from the public header.  In theory, it would be
>> possible to make the declaration conditional on
>> --enable-obsolete-rpc, but that would need quite a bit of shuffling
>> for a declaration that would be removed soon anyway.
>>
>> Despite the declaration of bindresvport6 in a public header file,
>> no implemention was provided by glibc, so this commit removes this
>> declaration without further changes.
>
> I think that bindresvport6 can be removed with a separate patch.

Sure, if we don't want to follow this approach.

Thanks,
Florian
H.J. Lu July 7, 2020, 5:28 p.m. UTC | #3
On Tue, Jul 7, 2020 at 10:04 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Tue, Jul 7, 2020 at 9:13 AM Florian Weimer via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> >>
> >> (This is a bit of a controversial change, but I'm not sure what our
> >> alternatives are, given the conflict with libtirpc.)
> >>
> >> bindresvport is mostly used along with Sun RPC functions.  libtirpc
> >> has its own implementation of it, and this implementation is
> >> is used when linking against libtirpc (under a separate symbol
> >> version).  The libtirpc implementation is superior because it
> >> provides a filter for well-known ports, addressing historic bug 456.
> >>
> >> Exceptions that use bindresvport without linking against libtirpc
> >> are torque (the resource manager) and the sign command in OBS
> >> (the build service).
> >
> > Have they been fixed?  Otherwise, they won't compile/link with glibc
> > 2.32.
>
> I suppose I can try to submit patches for them if this indeed the
> direction we want to move in.

I think it is a good thing when using with glibc, regardless of what
glibc does with
bindresvport.  We need to provide a solution before it is removed from
glibc API.

> The symbol clash with libtirpc is rather unfortunate.

True.

> >> This change turns bindresvport into a compatibility symbol because
> >> it is currently declared in a non-RPC header (<netinet/in.h>),
> >> and remvoes it from the public header.  In theory, it would be
> >> possible to make the declaration conditional on
> >> --enable-obsolete-rpc, but that would need quite a bit of shuffling
> >> for a declaration that would be removed soon anyway.
> >>
> >> Despite the declaration of bindresvport6 in a public header file,
> >> no implemention was provided by glibc, so this commit removes this
> >> declaration without further changes.
> >
> > I think that bindresvport6 can be removed with a separate patch.
>
> Sure, if we don't want to follow this approach.
>

I think 2 things are orthogonal.
Florian Weimer July 8, 2020, 12:10 p.m. UTC | #4
* H. J. Lu via Libc-alpha:

> On Tue, Jul 7, 2020 at 10:04 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > On Tue, Jul 7, 2020 at 9:13 AM Florian Weimer via Libc-alpha
>> > <libc-alpha@sourceware.org> wrote:
>> >>
>> >> (This is a bit of a controversial change, but I'm not sure what our
>> >> alternatives are, given the conflict with libtirpc.)
>> >>
>> >> bindresvport is mostly used along with Sun RPC functions.  libtirpc
>> >> has its own implementation of it, and this implementation is
>> >> is used when linking against libtirpc (under a separate symbol
>> >> version).  The libtirpc implementation is superior because it
>> >> provides a filter for well-known ports, addressing historic bug 456.
>> >>
>> >> Exceptions that use bindresvport without linking against libtirpc
>> >> are torque (the resource manager) and the sign command in OBS
>> >> (the build service).
>> >
>> > Have they been fixed?  Otherwise, they won't compile/link with glibc
>> > 2.32.
>>
>> I suppose I can try to submit patches for them if this indeed the
>> direction we want to move in.
>
> I think it is a good thing when using with glibc, regardless of what
> glibc does with
> bindresvport.  We need to provide a solution before it is removed from
> glibc API.

I've posted a patch that disentangles bindresvport from sunrpc, so that
we can revisit this issue at a later point:

  <https://sourceware.org/pipermail/libc-alpha/2020-July/115979.html>

Thanks,
Florian
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 839934d649..a28ce0047b 100644
--- a/NEWS
+++ b/NEWS
@@ -67,6 +67,9 @@  Deprecated and removed features, and other changes affecting compatibility:
 * ldconfig now defaults to the new format for ld.so.cache. glibc has
   already supported this format for almost 20 years.
 
+* The bindresvport function is no longer supported for new programs.
+  Applications that need this functions should link against libtirpc.
+
 Changes to build and runtime requirements:
 
 * powerpc64le requires GCC 7.4 or newer.  This is required for supporting
diff --git a/include/netinet/in.h b/include/netinet/in.h
index 5e377469e7..33e37553a5 100644
--- a/include/netinet/in.h
+++ b/include/netinet/in.h
@@ -3,7 +3,9 @@ 
 #include <inet/netinet/in.h>
 
 #ifndef _ISOMAC
-libc_hidden_proto (bindresvport)
+int __bindresvport (int __sockfd, struct sockaddr_in *__sock_in) __THROW;
+libc_hidden_proto (__bindresvport)
+
 libc_hidden_proto (in6addr_loopback)
 extern __typeof (in6addr_loopback) __in6addr_loopback;
 libc_hidden_proto (__in6addr_loopback)
diff --git a/inet/netinet/in.h b/inet/netinet/in.h
index f6355c7efe..a90e23e9d0 100644
--- a/inet/netinet/in.h
+++ b/inet/netinet/in.h
@@ -502,16 +502,6 @@  extern uint16_t htons (uint16_t __hostshort)
 
 #define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)
 
-#ifdef __USE_MISC
-/* Bind socket to a privileged IP port.  */
-extern int bindresvport (int __sockfd, struct sockaddr_in *__sock_in) __THROW;
-
-/* The IPv6 version of this function.  */
-extern int bindresvport6 (int __sockfd, struct sockaddr_in6 *__sock_in)
-     __THROW;
-#endif
-
-
 #define IN6_IS_ADDR_MC_NODELOCAL(a) \
 	(IN6_IS_ADDR_MULTICAST(a)					      \
 	 && ((((const uint8_t *) (a))[1] & 0xf) == 0x1))
diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
index da33c05101..357fa94fee 100644
--- a/sunrpc/bindrsvprt.c
+++ b/sunrpc/bindrsvprt.c
@@ -36,6 +36,7 @@ 
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <libc-lock.h>
+#include <shlib-compat.h>
 
 /*
  * Locks the static variables in this file.
@@ -46,7 +47,7 @@  __libc_lock_define_initialized (static, lock);
  * Bind a socket to a privileged IP port
  */
 int
-bindresvport (int sd, struct sockaddr_in *sin)
+__bindresvport (int sd, struct sockaddr_in *sin)
 {
   static short port;
   struct sockaddr_in myaddr;
@@ -107,4 +108,8 @@  bindresvport (int sd, struct sockaddr_in *sin)
 
   return res;
 }
-libc_hidden_def (bindresvport)
+libc_hidden_def (__bindresvport)
+
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_32)
+compat_symbol (libc, __bindresvport, bindresvport, GLIBC_2_0);
+#endif
diff --git a/sunrpc/clnt_tcp.c b/sunrpc/clnt_tcp.c
index 249e9c3584..534a75f2bb 100644
--- a/sunrpc/clnt_tcp.c
+++ b/sunrpc/clnt_tcp.c
@@ -148,7 +148,7 @@  clnttcp_create (struct sockaddr_in *raddr, u_long prog, u_long vers,
   if (*sockp < 0)
     {
       *sockp = __socket (AF_INET, SOCK_STREAM, IPPROTO_TCP);
-      (void) bindresvport (*sockp, (struct sockaddr_in *) 0);
+      (void) __bindresvport (*sockp, (struct sockaddr_in *) 0);
       if ((*sockp < 0)
 	  || (__connect (*sockp, (struct sockaddr *) raddr,
 			 sizeof (*raddr)) < 0))
diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c
index ee79b09b40..fcc90db434 100644
--- a/sunrpc/clnt_udp.c
+++ b/sunrpc/clnt_udp.c
@@ -185,7 +185,7 @@  __libc_clntudp_bufcreate (struct sockaddr_in *raddr, u_long program,
 	  goto fooy;
 	}
       /* attempt to bind to prov port */
-      (void) bindresvport (*sockp, (struct sockaddr_in *) 0);
+      (void) __bindresvport (*sockp, (struct sockaddr_in *) 0);
 #ifdef IP_RECVERR
       {
 	int on = 1;
diff --git a/sunrpc/svc_tcp.c b/sunrpc/svc_tcp.c
index efbdd22548..3c16e7a985 100644
--- a/sunrpc/svc_tcp.c
+++ b/sunrpc/svc_tcp.c
@@ -169,7 +169,7 @@  svctcp_create (int sock, u_int sendsize, u_int recvsize)
     }
   memset ((char *) &addr, 0, sizeof (addr));
   addr.sin_family = AF_INET;
-  if (bindresvport (sock, &addr))
+  if (__bindresvport (sock, &addr))
     {
       addr.sin_port = 0;
       (void) __bind (sock, (struct sockaddr *) &addr, len);
diff --git a/sunrpc/svc_udp.c b/sunrpc/svc_udp.c
index ae67c9b36a..69f75fdc19 100644
--- a/sunrpc/svc_udp.c
+++ b/sunrpc/svc_udp.c
@@ -140,7 +140,7 @@  svcudp_bufcreate (int sock, u_int sendsz, u_int recvsz)
     }
   memset ((char *) &addr, 0, sizeof (addr));
   addr.sin_family = AF_INET;
-  if (bindresvport (sock, &addr))
+  if (__bindresvport (sock, &addr))
     {
       addr.sin_port = 0;
       (void) __bind (sock, (struct sockaddr *) &addr, len);