This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Don't bind to registered ports in bindresvport
- From: Dan Nicholson <dbn dot lists at gmail dot com>
- To: Carlos O'Donell <carlos at systemhalted dot org>,Adam Conrad <adconrad at debian dot org>, Jeff Law <law at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Mon, 4 Jun 2012 08:25:40 -0700
- Subject: Re: [PATCH] Don't bind to registered ports in bindresvport
- References: <20120531153241.GA21672@buster.dwcab.com><CADZpyiyzShWCe1OgA0b+vZW6NQGT0Q9WUK4=jev5MRu7pUxKtQ@mail.gmail.com><20120601200113.GA24584@buster.dwcab.com><CADZpyizNLJkWKZKPhmb1GRZr33jsBR8wMkwE8EnN3nU8Mu_0cw@mail.gmail.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