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

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

On Tue, Jun 05, 2012 at 01:09:04PM -0700, Dan Nicholson wrote:
> On 6/5/12, Jeff Law <law@redhat.com> wrote:
> > On 06/04/2012 09:25 AM, Dan Nicholson wrote:
> >
> >>> 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):
> > It's definitely a problem.  103401 has enough state to get a sense of
> > the issue.
> >
> > I haven't looked at this for a while as the current plan didn't require
> > further action from me.
> >
> > Specifically for RHEL6 and older we were in the process of auditing the
> > major daemons to ensure they use portreserve.  While that's far from an
> > ideal solution (it's race prone and doesn't work well for daemon
> > restarts), it was deemed the best of a set of bad solutions.
> 
> Right, this is specifically the hack I think should go away.
> 
> > For RHEL7 and beyond we're planning to use capabilities within systemd
> > to reserve ports.  It's a step forward, but still far from ideal.
> 
> Definitely an improvement since systemd can reserve the ports more
> cleanly than an out of band process like portreserve, but still you're
> just plugging holes, IMO.
> 
> > Checking /etc/rpc every time something wants to open a rpc port might
> > get rather expensive and many ports are going to be avoided even though
> > those services aren't running.
> 
> It's actually /etc/services, but I don't think this is really
> expensive in the context. Since this is part of nss, it should
> essentially be grabbed from a static buffer in libc. In a call to ask
> libc to bind a reserved port for you, I don't think it's unexpected
> that it will query some part of nss.
> 
> > If someone installed an /etc/rpc with
> > reservations for every port in the IANA database where wouldn't be too
> > many ports left for bindresvport to use.
> 
> Originally I did write it that way, but Carlos suggested to change so
> that if all the ports are registered that it fall back to just
> grabbing the first open port. That's what the current patch does.
> There are actually 4 phases:
> 
> 1. Check between 600 & 1023, skipping open ports and ports registered
> in services
> 2. Check between 512 & 599, skipping open ports and ports registered in services
> 3. Check between 600 & 1023, skipping open ports
> 4. Check between 512 & 599, skipping open ports
> 
> If you get all the way through that, you'll get EADDRINUSE.
> 
> > We were looking at white/black lists of ports when we were pondering
> > fixing this in glibc itself.
> 
> Here I'm considering /etc/services as the black list. Maintaining a
> separate black list seems like it would eventually just become
> services given enough time. It's just a matter of time until someone
> files a bug asking to reserve 674 for acap or whatever. :) IMO, if you
> want a reserved port in the lower range, you get it in services.
> That's what it seems to be there for.
> 
> > However, I'll welcome this approach as just about anything would be
> > better than the current status quo.
> >
> > And for those who think everything needing a port between 600 & 1023
> > should start before that rpc services, that's just not feasible in the
> > real world.
> 
> Definitely agree with that.

Trying to revive this patch after some hiatus. Below if v3 of the patch
rebased. In this version, I simplified the memory handling by starting
with a fixed buffer and then just using malloc if necessary. The
behavior is still the same as before, although I had steps 2 and 3
swapped above.

There are 5 phases of operation:
 
1. Check between 600 & 1023, skipping open or registered ports.
2. Check between 600 & 1023, skipping open ports.
3. Check between 512 & 599, skipping open or registered.
4. Check between 512 & 599, skipping open ports.
5. Return EADDRINUSE.

I tested this a few times by narrowing the port ranges and binding to
ports in the range. It works correctly in my testing.

I think this would be a useful feature and nice to see in glibc-2.18.
Here's the updated patch:

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 programs providing a service
know what port to bind to and random programs know which ports 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 the first pass, avoid ports
that are registered in services. If no unregistered ports can be found,
fall back to taking the first randomly open port. On my fedora system,
295 of the 541 ports between 512 and 1023 are unregistered. This should
avoid registered ports on most typical systems.

2013-01-12  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.

diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
index e6a1b0b..70c95c4 100644
--- a/sunrpc/bindrsvprt.c
+++ b/sunrpc/bindrsvprt.c
@@ -36,6 +36,8 @@
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <bits/libc-lock.h>
+#include <rpc/types.h>
+#include <netdb.h>
 
 /*
  * Locks the static variables in this file.
@@ -80,29 +82,74 @@ bindresvport (int sd, struct sockaddr_in *sin)
 
   int nports = ENDPORT - startport + 1;
   int endport = ENDPORT;
+  bool_t chkport = TRUE;
+  struct servent serv;
+  char fixed_buf[1024];
+  char *buf = fixed_buf;
+  size_t bufsz = sizeof (fixed_buf);
 
   __libc_lock_lock (lock);
 
  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.  */
+	  while (__getservbyport_r (sin->sin_port, NULL, &serv, buf,
+				    bufsz, &s) == ERANGE)
+	    {
+	      char *tmpbuf = realloc (buf, bufsz * 2);
+	      if (tmpbuf)
+		{
+		  buf = tmpbuf;
+		  bufsz *= 2;
+		}
+	      else
+		break;
+	    }
+	}
+      if (s == NULL)
+	{
+	  /* Either we're not checking services or it's unregistered.  */
+	  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;
+	}
     }
 
+  if (buf != fixed_buf)
+    free (buf);
+
   __libc_lock_unlock (lock);
 
   return res;
-- 
1.7.7.6