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

["Carlos O'Donell" <carlos at systemhalted dot org>]

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

> 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?

> ?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.

> ?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.

> + ? ? ? __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.

> + ? ? ? ? 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

Cheers,
Carlos.