Re: [PATCH] Don't bind to registered ports in bindresvport

[Dan Nicholson <dbn dot lists at gmail dot com>]

On Thu, May 31, 2012 at 01:16:01PM -0400, Carlos O'Donell wrote:
> On Thu, May 31, 2012 at 11:32 AM, Dan Nicholson <dbn.lists@gmail.com> wrote:
> > When bindresvport binds to a random port, there's a good chance it will
> > pick one already registered in services. That's bad since the point of
> > services is to define well known ports so random programs know which
> > ones to avoid. The current behavior causes lots of downstream bugs and
> > requires hacky solutions like running programs early in boot to bind to
> > desired ports and handing them off when the actual services start.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=103401
> >
> > Let's just fix the problem at the source. On my fedora system, 295 of
> > the 541 ports between 512 and 1023 are unregistered. There's plenty of
> > space to pick a smarter port. If there are systems that require more
> > random ports than that, bindresvport is probably not the right API to
> > use.
> >
> > 2012-05-31 ?Dan Nicholson ?<dbn.lists@gmail.com>
> >
> > ? ? ? ?* sunrpc/bindrsvprt.c (bindresvport): Before binding the port,
> > ? ? ? ?make sure it's not registered in services.
> 
> This is an application management issue that needs to be handled by
> the distributions.
> 
> If a service depends on a port between 600-1023 then it must be
> started before *any* services that randomly use ports in that range
> e.g. via bindrsvprt.
> 
> Your patch attempts to encode a loose dependency via a modification to
> the behaviour of the implementation and that is unacceptable.
> 
> The dependency must be expressed at a higher level e.g. while managing
> the services.
> 
> A more acceptable patch might:
> 
> * Attempt to find a port for which a service is *not* reserved.
> * If no such port is found then fallback to the old behaviour.

OK, I don't entirely agree with your feelings on managing the issue at
the distro level. However, I see your point that it should fallback to
the old behavior. See below. I ran this through some tests by narrowing
the range down and it seems to work correctly. What do you think?

2012-06-01  Dan Nicholson  <dbn.lists@gmail.com>

	* sunrpc/bindrsvprt.c (bindresvport): Try to choose a port that
	is unregistered in services before falling back to the first
	open port.
---
 sunrpc/bindrsvprt.c |   46 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
index d493c9f..a12ee3e 100644
--- a/sunrpc/bindrsvprt.c
+++ b/sunrpc/bindrsvprt.c
@@ -35,6 +35,8 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
+#include <rpc/types.h>
+#include <netdb.h>
 
 /*
  * Bind a socket to a privileged IP port
@@ -74,24 +76,50 @@ bindresvport (int sd, struct sockaddr_in *sin)
 
   int nports = ENDPORT - startport + 1;
   int endport = ENDPORT;
+  bool_t chkport = TRUE;
+  struct servent serv;
+  char buf[1024];
  again:
   for (i = 0; i < nports; ++i)
     {
+      struct servent *s = NULL;
+
       sin->sin_port = htons (port++);
       if (port > endport)
 	port = startport;
-      res = __bind (sd, sin, sizeof (struct sockaddr_in));
-      if (res >= 0 || errno != EADDRINUSE)
-	break;
+      if (chkport)
+	/* Check to see if the port is registered in services. */
+	__getservbyport_r (sin->sin_port, NULL, &serv, buf, sizeof (buf), &s);
+      if (s == NULL)
+	{
+	  res = __bind (sd, sin, sizeof (struct sockaddr_in));
+	  if (res >= 0 || errno != EADDRINUSE)
+	    break;
+	}
     }
 
-  if (i == nports && startport != LOWPORT)
+  if (i == nports)
     {
-      startport = LOWPORT;
-      endport = STARTPORT - 1;
-      nports = STARTPORT - LOWPORT;
-      port = LOWPORT + port % (STARTPORT - LOWPORT);
-      goto again;
+      if (chkport)
+	{
+	  /* Try again, but don't bother checking services. */
+	  chkport = FALSE;
+	  if (startport != LOWPORT)
+	    port = (__getpid () % NPORTS) + STARTPORT;
+	  else
+	    port = LOWPORT + port % (STARTPORT - LOWPORT);
+	  goto again;
+	}
+
+      if (startport != LOWPORT)
+	{
+	  chkport = TRUE;
+	  startport = LOWPORT;
+	  endport = STARTPORT - 1;
+	  nports = STARTPORT - LOWPORT;
+	  port = LOWPORT + port % (STARTPORT - LOWPORT);
+	  goto again;
+	}
     }
 
   return res;
-- 
1.7.7.6