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

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

On Fri, Jun 01, 2012 at 11:03:53PM -0400, Carlos O'Donell wrote:
> On Fri, Jun 1, 2012 at 4:01 PM, Dan Nicholson <dbn.lists@gmail.com> wrote:
> >> 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?
> 
> Thanks, this looks better.
> 
> I'd like to see one of the distro maintainers review this also.
> 
> Could you please ask Adam Conrad from Canonical if he could review this?
> 
> http://sourceware.org/glibc/wiki/MAINTAINERS#Distribution_Maintainers

I've added him and Jeff Law since I know this has been an issue for both
distros. Here's the fedora bug again:

https://bugzilla.redhat.com/show_bug.cgi?id=103401

And here's the patch their using on Debian/Ubuntu currently to work
around the issue (header says it originates from OpenSUSE):

http://anonscm.debian.org/viewvc/pkg-glibc/glibc-package/trunk/debian/patches/any/local-bindresvport_blacklist.diff?view=markup

> > 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.
> 
> Do you have FSF copyright assignment?

No. I don't mind starting the process, though.

> > ?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];
> 
> A global static buffer of this size should come from malloc (and get
> allocated once).
> 
> There is precedence for using alloca, but I'd like to avoid that.

I reworked the patch to use malloc with a static buffer and check for
ERANGE from getservbyport_r.

> > ?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. */
> 
> Two spaces after period.

OK.

> > + ? ? ? __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. */
> 
> Two spaces after period.

OK. See updated patch below.

2012-06-04  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 |   63 +++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
index d493c9f..1b7a535 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,67 @@ bindresvport (int sd, struct sockaddr_in *sin)
 
   int nports = ENDPORT - startport + 1;
   int endport = ENDPORT;
+  bool_t chkport = TRUE;
+  struct servent serv;
+  static char *buf;
+  static size_t buflen = 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.  */
+	  if (buf == NULL)
+	    buf = malloc (buflen);
+	  if (buf != NULL)
+	    while (__getservbyport_r (sin->sin_port, NULL, &serv, buf,
+				      buflen, &s) == ERANGE)
+	      {
+		char *tmpbuf = realloc (buf, 2 * buflen);
+		if (tmpbuf)
+		  {
+		    buflen = 2 * buflen;
+		    buf = tmpbuf;
+		  }
+		else
+		  break;
+	      }
+	}
+      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