Skip to contentSkip to conversation

clnt_com_create: Restore backwards compatibility with the legacy glibc code.

PATCH In review Steve Dickson 2 participants 10 messages Apr 9, 2018 – Apr 11, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 clnt_com_create: Restore backwards compatibility with the legacy glibc code.

Commit 46e04a73 changed clnt_com_create() to avoid using reserved ports when creating the the CLIENT ptr. This change breaks backward compatibility with the legacy RPC code that was in glibc.

This patch reverts that commit to restore backwards compatibility

---
 src/rpc_soc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
src/rpc_soc.c +6 -4
index af6c482..ed0892a 100644
@@ -67,8 +67,6 @@
 
 extern mutex_t	rpcsoc_lock;
 
-extern int __binddynport(int fd);
-
 static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, rpcvers_t,
     int *, u_int, u_int, char *, int);
 static SVCXPRT *svc_com_create(int, u_int, u_int, char *);
@@ -147,8 +145,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, recvsz, tp, flags)
 	bindaddr.maxlen = bindaddr.len =  sizeof (struct sockaddr_in);
 	bindaddr.buf = raddr;
 
-	if (__binddynport(fd) == -1)
-		goto err;
+	bindresvport(fd, NULL);
 	cl = clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
 				sendsz, recvsz);
 	if (cl) {
@@ -316,6 +313,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
 	SVCXPRT *svc;
 	int madefd = FALSE;
 	int port;
+	struct sockaddr_in sin;
 
 	if ((nconf = __rpc_getconfip(netid)) == NULL) {
 		(void) syslog(LOG_ERR, "Could not get %s transport", netid);
@@ -332,6 +330,10 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd = TRUE;
 	}
 
+	memset(&sin, 0, sizeof sin);
+	sin.sin_family = AF_INET;
+	bindresvport(fd, &sin);
+	listen(fd, SOMAXCONN);
 	svc = svc_tli_create(fd, nconf, NULL, sendsize, recvsize);
 	(void) freenetconfigent(nconf);
 	if (svc == NULL) {
Signature · 1 line
2.14.3
#1 lore raw
Quoted 26 lines · Steve Dickson

=20
Commit 46e04a73 changed clnt_com_create() to avoid using reserved ports when creating the the CLIENT ptr. This change breaks backward compatibility with the legacy RPC code that was in glibc.
=20
This patch reverts that commit to restore backwards compatibility =20

---
src/rpc_soc.c | 10 ++++++----

1 file changed, 6 insertions(+), 4 deletions(-)

=20

src/rpc_soc.c +0 -0
index af6c482..ed0892a 100644
@@ -67,8 +67,6 @@

=20
extern mutex_t rpcsoc_lock;
=20

src/rpc_soc.c +0 -2
-extern int __binddynport(int fd);
-
Quoted 1 line

static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, =

rpcvers_t,

src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, sendsz, =

Quoted 3 lines
    int *, u_int, u_int, char *, int);

static SVCXPRT *svc_com_create(int, u_int, u_int, char *);

src/rpc_soc.c +0 -0
@@ -147,8 +145,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, =

recvsz, tp, flags)

src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, sendsz, = · line 145

Quoted 1 line
src/rpc_soc.c +0 -0
 	bindaddr.maxlen =3D bindaddr.len =3D  sizeof (struct =

sockaddr_in);

Quoted 14 lines
src/rpc_soc.c +0 -0
 	bindaddr.buf =3D raddr;

=20

src/rpc_soc.c +1 -2
-	if (__binddynport(fd) =3D=3D -1)
-		goto err;
+	bindresvport(fd, NULL);
	cl =3D clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
				sendsz, recvsz);
	if (cl) {
src/rpc_soc.c +1 -0
@@ -316,6 +313,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
 	SVCXPRT *svc;
 	int madefd =3D FALSE;
 	int port;
+	struct sockaddr_in sin;

=20

Quoted 2 lines
	if ((nconf =3D __rpc_getconfip(netid)) =3D=3D NULL) {
		(void) syslog(LOG_ERR, "Could not get %s transport", =

netid);

src/rpc_soc.c svc_com_create(fd, sendsize, recvsize, netid) · line 336

Quoted 8 lines
src/rpc_soc.c +0 -0
@@ -332,6 +330,10 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd =3D TRUE;
 	}

=20

src/rpc_soc.c +4 -0
+	memset(&sin, 0, sizeof sin);
+	sin.sin_family =3D AF_INET;
+	bindresvport(fd, &sin);
+	listen(fd, SOMAXCONN);

Why do we need to fix the server API too?

Quoted 8 lines
	svc =3D svc_tli_create(fd, nconf, NULL, sendsize, recvsize);
	(void) freenetconfigent(nconf);
	if (svc =3D=3D NULL) {

--=20
2.14.3
=20
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

in

Quoted 2 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever

#2 lore raw
Quoted 51 lines · Chuck Lever
Quoted 51 lines · Steve Dickson

Commit 46e04a73 changed clnt_com_create() to avoid using reserved ports when creating the the CLIENT ptr. This change breaks backward compatibility with the legacy RPC code that was in glibc.

This patch reverts that commit to restore backwards compatibility

---
src/rpc_soc.c | 10 ++++++----

1 file changed, 6 insertions(+), 4 deletions(-)
src/rpc_soc.c +2 -4
index af6c482..ed0892a 100644
@@ -67,8 +67,6 @@
 
 extern mutex_t	rpcsoc_lock;
 
-extern int __binddynport(int fd);
-
 static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, rpcvers_t,
    int *, u_int, u_int, char *, int);
 static SVCXPRT *svc_com_create(int, u_int, u_int, char *);
@@ -147,8 +145,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, recvsz, tp, flags)
 	bindaddr.maxlen = bindaddr.len =  sizeof (struct sockaddr_in);
 	bindaddr.buf = raddr;
 
-	if (__binddynport(fd) == -1)
-		goto err;
+	bindresvport(fd, NULL);
 	cl = clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
 				sendsz, recvsz);
 	if (cl) {
@@ -316,6 +313,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
 	SVCXPRT *svc;
 	int madefd = FALSE;
 	int port;
+	struct sockaddr_in sin;
 
 	if ((nconf = __rpc_getconfip(netid)) == NULL) {
 		(void) syslog(LOG_ERR, "Could not get %s transport", netid);
@@ -332,6 +330,10 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd = TRUE;
 	}
 
Quoted 6 lines

src/rpc_soc.c svc_com_create(fd, sendsize, recvsize, netid) · line 336

Quoted 4 lines
src/rpc_soc.c +4 -0
+	memset(&sin, 0, sizeof sin);
+	sin.sin_family = AF_INET;
+	bindresvport(fd, &sin);
+	listen(fd, SOMAXCONN);

Why do we need to fix the server API too?

I thought about this... and I'm thinking there is more of an exception of server listening on reserve ports than clients using using reserve ports.

Plus this make us completely backwards compatibility which in the long run I think is the right place to be.

steved.

Quoted 18 lines
Quoted 12 lines
src/rpc_soc.c +0 -0
 	svc = svc_tli_create(fd, nconf, NULL, sendsize, recvsize);
 	(void) freenetconfigent(nconf);
 	if (svc == NULL) {
Signature · 6 lines
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever

#3 lore raw
Quoted 10 lines · Steve Dickson

=20
=20
=20

Quoted 3 lines · Chuck Lever

=20
=20

Quoted 1 line

On Apr 9, 2018, at 11:28 AM, Steve Dickson <[email protected]> =

Quoted 16 lines · wrote:
Quoted 16 lines
Quoted 16 lines

=20
Commit 46e04a73 changed clnt_com_create() to avoid using reserved ports when creating the the CLIENT ptr. This change breaks backward compatibility with the legacy RPC code that was in glibc.
=20
This patch reverts that commit to restore backwards compatibility =20

---
src/rpc_soc.c | 10 ++++++----

1 file changed, 6 insertions(+), 4 deletions(-)

=20

src/rpc_soc.c +0 -1
index af6c482..ed0892a 100644
--- a/src/rpc_soc.c
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

+++ b/src/rpc_soc.c

src/rpc_soc.c +0 -0
@@ -67,8 +67,6 @@

=20
extern mutex_t rpcsoc_lock;
=20

src/rpc_soc.c +0 -2
-extern int __binddynport(int fd);
-

static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, =

rpcvers_t,

Quoted 3 lines
Quoted 3 lines

src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, =

Quoted 3 lines
   int *, u_int, u_int, char *, int);

static SVCXPRT *svc_com_create(int, u_int, u_int, char *);

src/rpc_soc.c +0 -0
@@ -147,8 +145,7 @@ clnt_com_create(raddr, prog, vers, sockp, =

sendsz, recvsz, tp, flags)

Quoted 1 line
Quoted 1 line

src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, = · line 145

Quoted 1 line
src/rpc_soc.c +0 -0
 	bindaddr.maxlen =3D bindaddr.len =3D  sizeof (struct =

sockaddr_in);

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
src/rpc_soc.c +0 -0
 	bindaddr.buf =3D raddr;

=20

src/rpc_soc.c +1 -2
-	if (__binddynport(fd) =3D=3D -1)
-		goto err;
+	bindresvport(fd, NULL);
	cl =3D clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
				sendsz, recvsz);
	if (cl) {
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
src/rpc_soc.c +1 -0
@@ -316,6 +313,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
 	SVCXPRT *svc;
 	int madefd =3D FALSE;
 	int port;
+	struct sockaddr_in sin;

=20

	if ((nconf =3D __rpc_getconfip(netid)) =3D=3D NULL) {
		(void) syslog(LOG_ERR, "Could not get %s transport", =

netid);

Quoted 10 lines
Quoted 10 lines

src/rpc_soc.c svc_com_create(fd, sendsize, recvsize, netid) · line 336

Quoted 8 lines
src/rpc_soc.c +0 -0
@@ -332,6 +330,10 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd =3D TRUE;
 	}

=20

src/rpc_soc.c +4 -0
+	memset(&sin, 0, sizeof sin);
+	sin.sin_family =3D AF_INET;
+	bindresvport(fd, &sin);
+	listen(fd, SOMAXCONN);

=20
Why do we need to fix the server API too?

Quoted 3 lines

I thought about this... and I'm thinking there is more of an exception of server listening on reserve ports than clients using using reserve ports.

Hi Steve-

I think you mean that there are fewer legacy servers than there are legacy clients? I don't see how that
matters: There's no benefit at all for having a server listen on a dynamically-assigned privileged port. Nothing can be broken if the server API uses an ephemeral port.

Quoted 2 lines

Plus this make us completely backwards compatibility which in the long run I think is the right place to be.

Philosophically I agree that backwards compatibility is good, but I think you're "going to hell with the joke" as they say.

The point of backwards compatibility is that we don't want to break applications that depend on some behavior. There can't be any applications that depend on this behavior because there's no functional benefit to it, only a downside, which we have no reason not to fix.

I'm going to firmly object on this one, unless you have a clearly documented breakage that requires the legacy server API to use bindresvport(3).

Also, it would be great to get a man page update on the legacy clnt API that documents the behavior when a root caller uses that API. That gives some guarantee that it doesn't get changed again inadvertently.

Quoted 12 lines

steved.
=20

Quoted 10 lines

=20
=20

Quoted 8 lines
	svc =3D svc_tli_create(fd, nconf, NULL, sendsize, recvsize);
	(void) freenetconfigent(nconf);
	if (svc =3D=3D NULL) {

--=20
2.14.3
=20
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

in

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

=20
--
Chuck Lever

--
Chuck Lever

#4 lore raw
Quoted 85 lines · Chuck Lever
Quoted 64 lines · Steve Dickson
Quoted 51 lines · Chuck Lever
Quoted 51 lines · Steve Dickson

Commit 46e04a73 changed clnt_com_create() to avoid using reserved ports when creating the the CLIENT ptr. This change breaks backward compatibility with the legacy RPC code that was in glibc.

This patch reverts that commit to restore backwards compatibility

---
src/rpc_soc.c | 10 ++++++----

1 file changed, 6 insertions(+), 4 deletions(-)
src/rpc_soc.c +2 -4
index af6c482..ed0892a 100644
@@ -67,8 +67,6 @@
 
 extern mutex_t	rpcsoc_lock;
 
-extern int __binddynport(int fd);
-
 static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, rpcvers_t,
   int *, u_int, u_int, char *, int);
 static SVCXPRT *svc_com_create(int, u_int, u_int, char *);
@@ -147,8 +145,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, recvsz, tp, flags)
 	bindaddr.maxlen = bindaddr.len =  sizeof (struct sockaddr_in);
 	bindaddr.buf = raddr;
 
-	if (__binddynport(fd) == -1)
-		goto err;
+	bindresvport(fd, NULL);
 	cl = clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
 				sendsz, recvsz);
 	if (cl) {
@@ -316,6 +313,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
 	SVCXPRT *svc;
 	int madefd = FALSE;
 	int port;
+	struct sockaddr_in sin;
 
 	if ((nconf = __rpc_getconfip(netid)) == NULL) {
 		(void) syslog(LOG_ERR, "Could not get %s transport", netid);
@@ -332,6 +330,10 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd = TRUE;
 	}
 
Quoted 6 lines

src/rpc_soc.c svc_com_create(fd, sendsize, recvsize, netid) · line 336

Quoted 4 lines
src/rpc_soc.c +4 -0
+	memset(&sin, 0, sizeof sin);
+	sin.sin_family = AF_INET;
+	bindresvport(fd, &sin);
+	listen(fd, SOMAXCONN);

Why do we need to fix the server API too?

Quoted 3 lines

I thought about this... and I'm thinking there is more of an exception of server listening on reserve ports than clients using using reserve ports.

Hi Steve-

I think you mean that there are fewer legacy servers than there are legacy clients? I don't see how that
matters: There's no benefit at all for having a server listen on a dynamically-assigned privileged port. Nothing can be broken if the server API uses an ephemeral port.

Quoted 2 lines

Plus this make us completely backwards compatibility which in the long run I think is the right place to be.

Philosophically I agree that backwards compatibility is good, but I think you're "going to hell with the joke" as they say.

Quoted 5 lines

The point of backwards compatibility is that we don't
want to break applications that depend on some behavior.
There can't be any applications that depend on this
behavior because there's no functional benefit to it,
only a downside, which we have no reason not to fix.The downside is we are changing an API that has

been established for since the 80's... What good
could that possibly do WRT legacy servers?

Quoted 4 lines

I'm going to firmly object on this one, unless you have a clearly documented breakage that requires the legacy server API to use bindresvport(3).

Here is what the man page says "If the socket is not bound to a local TCP port, then this routine binds it to an arbitrary port."

The point being it also does not talk about creating a listening connection either... Changing the (undocumented) API like this can cause nothing but problems... IMHO...

Basically, not making this change will cause a fork with all the major distros since it very import for them to be backward compatible esp in enterprise worlds. Upstream not so much... Who really uses raw upstream bits in a stable environment... understood... But this brings me to my point.

What problem is being fixed by changing an 20+ year API? Where are the bug reports that this change is needed or wanted?

Quoted 5 lines

Also, it would be great to get a man page update on the legacy clnt API that documents the behavior when a root caller uses that API. That gives some guarantee that it doesn't get changed again inadvertently.

I need to look, but I think glibc still "owns" the man pages...

steved.

#5 lore raw
Quoted 10 lines · Steve Dickson

=20
=20
=20

Quoted 3 lines · Chuck Lever

=20
=20

Quoted 1 line

On Apr 9, 2018, at 11:58 AM, Steve Dickson <[email protected]> =

Quoted 7 lines · wrote:
Quoted 7 lines
Quoted 7 lines

=20
=20
=20

Quoted 3 lines · Chuck Lever

=20
=20

Quoted 1 line

On Apr 9, 2018, at 11:28 AM, Steve Dickson <[email protected]> =

Quoted 16 lines · wrote:
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines

=20
Commit 46e04a73 changed clnt_com_create() to avoid using reserved ports when creating the the CLIENT ptr. This change breaks backward compatibility with the legacy RPC code that was in glibc.
=20
This patch reverts that commit to restore backwards compatibility =20

---
src/rpc_soc.c | 10 ++++++----

1 file changed, 6 insertions(+), 4 deletions(-)

=20

src/rpc_soc.c +0 -1
index af6c482..ed0892a 100644
--- a/src/rpc_soc.c
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

+++ b/src/rpc_soc.c

src/rpc_soc.c +0 -0
@@ -67,8 +67,6 @@

=20
extern mutex_t rpcsoc_lock;
=20

src/rpc_soc.c +0 -2
-extern int __binddynport(int fd);
-

static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, =

rpcvers_t,

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, =

Quoted 3 lines

int *, u_int, u_int, char *, int);
static SVCXPRT *svc_com_create(int, u_int, u_int, char *);

src/rpc_soc.c +0 -0
@@ -147,8 +145,7 @@ clnt_com_create(raddr, prog, vers, sockp, =

sendsz, recvsz, tp, flags)

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

src/rpc_soc.c clnt_com_create(raddr, prog, vers, sockp, = · line 145

Quoted 1 line
src/rpc_soc.c +0 -0
 	bindaddr.maxlen =3D bindaddr.len =3D  sizeof (struct =

sockaddr_in);

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
src/rpc_soc.c +0 -0
 	bindaddr.buf =3D raddr;

=20

src/rpc_soc.c +1 -2
-	if (__binddynport(fd) =3D=3D -1)
-		goto err;
+	bindresvport(fd, NULL);
	cl =3D clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
				sendsz, recvsz);
	if (cl) {
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
src/rpc_soc.c +1 -0
@@ -316,6 +313,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
 	SVCXPRT *svc;
 	int madefd =3D FALSE;
 	int port;
+	struct sockaddr_in sin;

=20

	if ((nconf =3D __rpc_getconfip(netid)) =3D=3D NULL) {
		(void) syslog(LOG_ERR, "Could not get %s transport", =

netid);

Quoted 36 lines
Quoted 26 lines
Quoted 10 lines
Quoted 10 lines

src/rpc_soc.c svc_com_create(fd, sendsize, recvsize, netid) · line 336

Quoted 8 lines
src/rpc_soc.c +0 -0
@@ -332,6 +330,10 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd =3D TRUE;
 	}

=20

src/rpc_soc.c +4 -0
+	memset(&sin, 0, sizeof sin);
+	sin.sin_family =3D AF_INET;
+	bindresvport(fd, &sin);
+	listen(fd, SOMAXCONN);

=20
Why do we need to fix the server API too?

Quoted 3 lines

I thought about this... and I'm thinking there is more of an exception of server listening on reserve ports than clients using using reserve ports.

=20
Hi Steve-
=20
I think you mean that there are fewer legacy servers than there are legacy clients? I don't see how that
matters: There's no benefit at all for having a server listen on a dynamically-assigned privileged port. Nothing can be broken if the server API uses an ephemeral port.
=20
=20

Quoted 2 lines

Plus this make us completely backwards compatibility which in the long run I think is the right place to be.

Quoted 10 lines

=20
Philosophically I agree that backwards compatibility is good, but I think you're "going to hell with the joke" as they say.
=20
The point of backwards compatibility is that we don't want to break applications that depend on some behavior. There can't be any applications that depend on this behavior because there's no functional benefit to it, only a downside, which we have no reason not to fix.

Quoted 3 lines

The downside is we are changing an API that has=20 been established for since the 80's... What good could that possibly do WRT legacy servers?

That's the wrong question, IMO. It won't hurt legacy servers at all, and will benefit everyone else.

Quoted 9 lines
Quoted 3 lines

I'm going to firmly object on this one, unless you have a clearly documented breakage that requires the legacy server API to use bindresvport(3).

Here is what the man page says "If the socket is not bound to a=20 local TCP port, then this routine binds it to an arbitrary port." =20
The point being it also does not talk about creating a=20 listening connection either... Changing the (undocumented) API like this can cause nothing but problems... IMHO...

Please show me one specific breakage that will result in removing bindresvport from svc_com_create. Just one, and I promise I will crawl back into my hole.

In case it wasn't clear, my preference is for a patch that reverts only the removal of bindresvport(3) from clnt_com_create. The svc_com_create change is safe and should remain unreverted.

I don't like reverting the clnt_com_create change, but I won't object to it, and there is clear evidence that some old programs are inconvenienced by that change.

Quoted 3 lines

Basically, not making this change will cause a fork with=20 all the major distros since it very import for them to be=20 backward compatible esp in enterprise worlds.

That sounds like an overstatement. Who claims they won't take libtirpc with an unprivileged svc_com_create? I would really like to understand why.

Quoted 5 lines

Upstream not=20
so much... Who really uses raw upstream bits in a stable environment... understood... But this brings me to my point. =20
What problem is being fixed by changing an 20+ year API?=20

The problem is legacy clients and servers are squatting on ports that are assigned to other network services. These patches mitigate that problem. There is more to be done.

The backwards compatibility issue is that some old servers believe that clients have to use a privileged source port to show that they are an authorized RPC consumer. We address that by reverting clnt_com_create to use bindresvport(3), although we can easily decide this is a security bug that is worth forcing legacy API consumers to fix their usage.

There is no similar backwards compatibility issue for clients talking to servers. That's just a fact about how RPC operates: the client uses rpcbind to discover the server's port. After that, there's no additional check to see if that service port is less than 1024.

Quoted 2 lines

Where are the bug reports that this change is needed or wanted?=20

I was presented with a list of five or more bug reports from various distributors that go back to almost 2000 where RPC consumers have caused issues occupying IANA- assigned reserved ports.

=
https://sourceforge.net/p/libtirpc/mailman/libtirpc-devel/?viewmonth=3D201=
801&viewday=3D12&style=3Dflat

The bottom item on that page is Guillem's e-mail that cites bug reports on this issue.

Quoted 6 lines
Quoted 4 lines

Also, it would be great to get a man page update on the legacy clnt API that documents the behavior when a root caller uses that API. That gives some guarantee that it doesn't get changed again inadvertently.

I need to look, but I think glibc still "owns" the man pages...=20

That should be fixed if that's true. Since RPC is to be removed from glibc I can't think why they'd want to keep the RPC man pages.

However, libtirpc has a bunch of man pages for the legacy APIs. At least those can be updated.

--
Chuck Lever

#6 lore raw
Quoted 111 lines · Chuck Lever
Quoted 92 lines · Steve Dickson
Quoted 85 lines · Chuck Lever
Quoted 64 lines · Steve Dickson
Quoted 51 lines · Chuck Lever
Quoted 51 lines · Steve Dickson

Commit 46e04a73 changed clnt_com_create() to avoid using reserved ports when creating the the CLIENT ptr. This change breaks backward compatibility with the legacy RPC code that was in glibc.

This patch reverts that commit to restore backwards compatibility

---
src/rpc_soc.c | 10 ++++++----

1 file changed, 6 insertions(+), 4 deletions(-)
src/rpc_soc.c +2 -4
index af6c482..ed0892a 100644
@@ -67,8 +67,6 @@
 
 extern mutex_t	rpcsoc_lock;
 
-extern int __binddynport(int fd);
-
 static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, rpcvers_t,
  int *, u_int, u_int, char *, int);
 static SVCXPRT *svc_com_create(int, u_int, u_int, char *);
@@ -147,8 +145,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, recvsz, tp, flags)
 	bindaddr.maxlen = bindaddr.len =  sizeof (struct sockaddr_in);
 	bindaddr.buf = raddr;
 
-	if (__binddynport(fd) == -1)
-		goto err;
+	bindresvport(fd, NULL);
 	cl = clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
 				sendsz, recvsz);
 	if (cl) {
@@ -316,6 +313,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
 	SVCXPRT *svc;
 	int madefd = FALSE;
 	int port;
+	struct sockaddr_in sin;
 
 	if ((nconf = __rpc_getconfip(netid)) == NULL) {
 		(void) syslog(LOG_ERR, "Could not get %s transport", netid);
@@ -332,6 +330,10 @@ svc_com_create(fd, sendsize, recvsize, netid)
 		madefd = TRUE;
 	}
 
Quoted 6 lines

src/rpc_soc.c svc_com_create(fd, sendsize, recvsize, netid) · line 336

Quoted 4 lines
src/rpc_soc.c +4 -0
+	memset(&sin, 0, sizeof sin);
+	sin.sin_family = AF_INET;
+	bindresvport(fd, &sin);
+	listen(fd, SOMAXCONN);

Why do we need to fix the server API too?

Quoted 3 lines

I thought about this... and I'm thinking there is more of an exception of server listening on reserve ports than clients using using reserve ports.

Hi Steve-

I think you mean that there are fewer legacy servers than there are legacy clients? I don't see how that
matters: There's no benefit at all for having a server listen on a dynamically-assigned privileged port. Nothing can be broken if the server API uses an ephemeral port.

Quoted 2 lines

Plus this make us completely backwards compatibility which in the long run I think is the right place to be.

Philosophically I agree that backwards compatibility is good, but I think you're "going to hell with the joke" as they say.

Quoted 8 lines
Quoted 5 lines

The point of backwards compatibility is that we don't want to break applications that depend on some behavior. There can't be any applications that depend on this behavior because there's no functional benefit to it, only a downside, which we have no reason not to fix.

The downside is we are changing an API that has
been established for since the 80's... What good
could that possibly do WRT legacy servers?

That's the wrong question, IMO. It won't hurt legacy servers at all, and will benefit everyone else.

Quoted 6 lines
Quoted 3 lines

I'm going to firmly object on this one, unless you have a clearly documented breakage that requires the legacy server API to use bindresvport(3).

Here is what the man page says "If the socket is not bound to a local TCP port, then this routine binds it to an arbitrary port."

Quoted 7 lines
Quoted 3 lines

The point being it also does not talk about creating a listening connection either... Changing the (undocumented) API like this can cause nothing but problems... IMHO...

Please show me one specific breakage that will result in removing bindresvport from svc_com_create. Just one, and I promise I will crawl back into my hole.

The patch also restores the listen() which is
something I can see a server apps depending on.

Quoted 9 lines

In case it wasn't clear, my preference is for a patch that reverts only the removal of bindresvport(3) from clnt_com_create. The svc_com_create change is safe and should remain unreverted.

I don't like reverting the clnt_com_create change, but I won't object to it, and there is clear evidence that some old programs are inconvenienced by that change.

I do not want to be in that position again... I just don't want to wait around to see what breaks.

Quoted 9 lines
Quoted 5 lines

Basically, not making this change will cause a fork with all the major distros since it very import for them to be backward compatible esp in enterprise worlds.

That sounds like an overstatement. Who claims they won't take libtirpc with an unprivileged svc_com_create? I would really like to understand why.

I'm just assuming most distros want to be backward compatible and not break legacy apps... Maybe I'm wrong...

Quoted 11 lines
Quoted 7 lines

Upstream not
so much... Who really uses raw upstream bits in a stable environment... understood... But this brings me to my point.

What problem is being fixed by changing an 20+ year API?

The problem is legacy clients and servers are squatting on ports that are assigned to other network services. These patches mitigate that problem. There is more to be done.

Any examples of these... The only one I know is rpcbind which can be fixed in another ways....

Quoted 8 lines

The backwards compatibility issue is that some old servers believe that clients have to use a privileged source port to show that they are an authorized RPC consumer. We address that by reverting clnt_com_create to use bindresvport(3), although we can easily decide this is a security bug that is worth forcing legacy API consumers to fix their usage.

Quoted 5 lines

There is no similar backwards compatibility issue for clients talking to servers. That's just a fact about how RPC operates: the client uses rpcbind to discover the server's port. After that, there's no additional check to see if that service port is less than 1024.

Well its going to be hard for the client to talk to the server if the server is no longer listening ;-)

Quoted 20 lines
Quoted 4 lines

Where are the bug reports that this change is needed or wanted?

I was presented with a list of five or more bug reports from various distributors that go back to almost 2000 where RPC consumers have caused issues occupying IANA- assigned reserved ports.

https://sourceforge.net/p/libtirpc/mailman/libtirpc-devel/?viewmonth=201801&viewday=12&style=flat

The bottom item on that page is Guillem's e-mail that cites bug reports on this issue.

Quoted 4 lines
Quoted 4 lines

Also, it would be great to get a man page update on the legacy clnt API that documents the behavior when a root caller uses that API. That gives some guarantee that it doesn't get changed again inadvertently.

Quoted 6 lines
Quoted 2 lines

I need to look, but I think glibc still "owns" the man pages...

That should be fixed if that's true. Since RPC is to be removed from glibc I can't think why they'd want to keep the RPC man pages.

I'll talk to the glibc people to what they are planning...

steved.

Quoted 10 lines

However, libtirpc has a bunch of man pages for the legacy APIs. At least those can be updated.

--
Chuck Lever

#7 lore raw
Quoted 19 lines · Steve Dickson
Quoted 14 lines · Chuck Lever
Quoted 13 lines · Steve Dickson
Quoted 3 lines · Chuck Lever

I'm going to firmly object on this one, unless you have a clearly documented breakage that requires the legacy server API to use bindresvport(3).

Here is what the man page says "If the socket is not bound to a local TCP port, then this routine binds it to an arbitrary port."

The point being it also does not talk about creating a listening connection either... Changing the (undocumented) API like this can cause nothing but problems... IMHO...

Quoted 5 lines
Quoted 3 lines

Please show me one specific breakage that will result in removing bindresvport from svc_com_create. Just one, and I promise I will crawl back into my hole.

The patch also restores the listen() which is
something I can see a server apps depending on.

I'm not clear there's a problem there. Did you find that svc_tli_create() was not already doing a listen?

Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

Basically, not making this change will cause a fork with all the major distros since it very import for them to be backward compatible esp in enterprise worlds.

That sounds like an overstatement. Who claims they won't take libtirpc with an unprivileged svc_com_create? I would really like to understand why.

I'm just assuming most distros want to be backward compatible and not break legacy apps... Maybe I'm wrong...

Of course they don't want to break legacy apps. I just don't see any backwards compatibility requirement here: servers never need a reserved port, they need either a specific port, or any port.

Therefore the libtirpc server-side APIs should not use bindresvport.

Quoted 11 lines
Quoted 9 lines
Quoted 5 lines

Upstream not
so much... Who really uses raw upstream bits in a stable environment... understood... But this brings me to my point.

What problem is being fixed by changing an 20+ year API?

The problem is legacy clients and servers are squatting on ports that are assigned to other network services. These patches mitigate that problem. There is more to be done.

Any examples of these... The only one I know is rpcbind which can be fixed in another ways....

Yes, as you and I discussed with Trond, statd can squat on a reserved port for it's downcall client to lockd. The fix Trond prefers is for this downcall service to use an AF_LOCAL socket instead.

We should also update bindresvport(3) to do more to avoid selecting ports that appear in /etc/services. This will cover user apps that use the RPC library API.

--
Chuck Lever

#8 lore raw
Quoted 19 lines · Chuck Lever
Quoted 19 lines · Steve Dickson
Quoted 14 lines · Chuck Lever
Quoted 13 lines · Steve Dickson
Quoted 3 lines · Chuck Lever

I'm going to firmly object on this one, unless you have a clearly documented breakage that requires the legacy server API to use bindresvport(3).

Here is what the man page says "If the socket is not bound to a local TCP port, then this routine binds it to an arbitrary port."

The point being it also does not talk about creating a listening connection either... Changing the (undocumented) API like this can cause nothing but problems... IMHO...

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Please show me one specific breakage that will result in removing bindresvport from svc_com_create. Just one, and I promise I will crawl back into my hole.

The patch also restores the listen() which is
something I can see a server apps depending on.

I'm not clear there's a problem there. Did you find that svc_tli_create() was not already doing a listen?

yeah I didn't see that...

Quoted 30 lines
Quoted 11 lines
Quoted 9 lines
Quoted 5 lines

Basically, not making this change will cause a fork with all the major distros since it very import for them to be backward compatible esp in enterprise worlds.

That sounds like an overstatement. Who claims they won't take libtirpc with an unprivileged svc_com_create? I would really like to understand why.

I'm just assuming most distros want to be backward compatible and not break legacy apps... Maybe I'm wrong...

Of course they don't want to break legacy apps. I just don't see any backwards compatibility requirement here: servers never need a reserved port, they need either a specific port, or any port.

Therefore the libtirpc server-side APIs should not use bindresvport.

Quoted 9 lines
Quoted 9 lines
Quoted 5 lines

Upstream not
so much... Who really uses raw upstream bits in a stable environment... understood... But this brings me to my point.

What problem is being fixed by changing an 20+ year API?

The problem is legacy clients and servers are squatting on ports that are assigned to other network services. These patches mitigate that problem. There is more to be done.

Quoted 7 lines
Quoted 2 lines

Any examples of these... The only one I know is rpcbind which can be fixed in another ways....

Yes, as you and I discussed with Trond, statd can squat on a reserved port for it's downcall client to lockd. The fix Trond prefers is for this downcall service to use an AF_LOCAL socket instead.

But it is careful not to used any ports in /etc/services

steved.

Quoted 11 lines

We should also update bindresvport(3) to do more to avoid selecting ports that appear in /etc/services. This will cover user apps that use the RPC library API.

--
Chuck Lever

#9 lore raw
Quoted 20 lines · Steve Dickson
Quoted 13 lines · Chuck Lever
Quoted 13 lines · Steve Dickson
Quoted 10 lines
Quoted 6 lines

Upstream not
so much... Who really uses raw upstream bits in a stable environment... understood... But this brings me to my point.

What problem is being fixed by changing an 20+ year API?

The problem is legacy clients and servers are squatting on ports that are assigned to other network services. These patches mitigate that problem. There is more to be done.

Quoted 8 lines
Quoted 7 lines
Quoted 2 lines

Any examples of these... The only one I know is rpcbind which can be fixed in another ways....

Yes, as you and I discussed with Trond, statd can squat on a reserved port for it's downcall client to lockd. The fix Trond prefers is for this downcall service to use an AF_LOCAL socket instead.

But it is careful not to used any ports in /etc/services

There are two sides to this reserved port coin.

- The "heads" side is that long-lived clients shouldn't squat on a port that is allocated to another service. This includes IANA-allocated ports below and above 1024.

- The "tails" side is that avoiding IANA-allocated ports below 1024 reduces the already-small pool of reserved ports that can be selected.

The "heads" part can be addressed by making bindresvport(3) act more like the current statd downcall client. Even if many clients are short-lived, we don't have any control over how long clients live, and even a short-lived client can block reliable start-up of well-known services temporarily, leading to annoying non-determinant behavior. Thus making this API work better is still important. (This is what Guillem was trying to address in January).

Moreover we have to ensure that bindresvport(3) doesn't fail to find a port just because the only ports that happen to be available are IANA-allocated. Thus we should address the "tails" part of this too, and make libtirpc uses this scarce shared resource economically. (This is the part Guillem's patch didn't address, as many reviewers pointed out).

There are some applications that use reserved ports today that don't need to:

- Any listener, for example, does not need an arbitrarily- assigned reserved port. By using __binddynport, the dynamically assigned port avoids all IANA-allocated ports (even IANA-allocated ports above 1024).

- Many client applications, even if they are short-lived, do not require a privileged source port. And to avoid an IANA- allocated port, they should likewise use __binddynport. That really should be the default behavior of the CLNT interfaces, because bindresvport(3) is available for the few clients that need a privileged source port.

- As you pointed out, applications that use TCP will leave a port in TIME_WAIT, which takes that source port out of the pool for 120 seconds. That's painful if the port is reserved.

Fixing these applications helps to address "tails". By changing the libtirpc client APIs, we address the second and third points for a fleet of RPC clients.

I don't have much empathy for YP:
- The use of NIS/YP is deprecated
- The use of a privileged source port is security theater
- The API behavior YP relies on is not documented
- There already exist patches available to make YP do this right

In this particular case, although backwards compatibility is important, IMO reliable system operation is a higher priority.

--
Chuck Lever