On Fri, Jan 07, 2011 at 08:48:20AM -0800, Jeremy Evans wrote:
> On 01/07 09:31, Nicholas Marriott wrote:
> > On Thu, Jan 06, 2011 at 03:32:17PM -0800, Jeremy Evans wrote:
> > > This patch adds unix datagram socket support to nc(1). It's basically
> > > the same patch I sent last June (see
> > > http://marc.info/?l=openbsd-tech&m=127627296925965&w=2), but updated
> > > for -current.
> > >
> > > Tested on amd64. Doesn't appear to cause any regressions to existing
> > > support, tested with unix stream and IP stream and datagram sockets.
> > > Looking for OKs.
> > >
> > > Jeremy
> >
> > Hmm, ISTR I meant to look at this ages ago but it got lost, sorry.
> >
> > So you are overloading -u to mean UDP without -l and datagram with -l?
> > I guess this makes sense, but we need man page changes?
>
> If you mean -U instead of -l, yes. -u without -U is IP UDP, -u with -U
> is unix datagram. The man page doesn't say you can't use -u and -U
> together, and it doesn't say that -U means unix stream sockets, though
> that is currently all that -U supports. However, I think that making
> some clarifications to the man page would helpful.
Yes, sorry, I meant -U.
This mostly looks fine to me, a few comments:
I don't much like using mktemp at all. How about just plain requiring -s
with -Uu?
Do you actually hit the ENOBUFS condition in atomicio.c?
>
> Responses inline and new diff at the end.
>
> > > Index: netcat.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.bin/nc/netcat.c,v
> > > retrieving revision 1.98
> > > diff -u -p -r1.98 netcat.c
> > > --- netcat.c 3 Jul 2010 04:44:51 -0000 1.98
> > > +++ netcat.c 6 Jan 2011 21:48:04 -0000
> > > @@ -89,6 +89,7 @@ u_int rtableid;
> > > int timeout = -1;
> > > int family = AF_UNSPEC;
> > > char *portlist[PORT_MAX+1];
> > > +char *unix_dg_tmp_socket;
> > >
> > > void atelnet(int, unsigned char *, unsigned int);
> > > void build_ports(char *);
> > > @@ -99,6 +100,7 @@ int remote_connect(const char *, const c
> > > int socks_connect(const char *, const char *, struct addrinfo,
> > > const char *, const char *, struct addrinfo, int, const char *);
> > > int udptest(int);
> > > +int unix_bind(char *);
> > > int unix_connect(char *);
> > > int unix_listen(char *);
> > > void set_common_sockopts(int);
> > > @@ -241,8 +243,6 @@ main(int argc, char *argv[])
> > >
> > > /* Cruft to make sure options are clean, and used properly. */
> > > if (argv[0] && !argv[1] && family == AF_UNIX) {
> > > - if (uflag)
> > > - errx(1, "cannot use -u and -U");
> > > host = argv[0];
> > > uport = NULL;
> > > } else if (argv[0] && !argv[1]) {
> > > @@ -265,6 +265,18 @@ main(int argc, char *argv[])
> > > if (!lflag && kflag)
> > > errx(1, "must use -l with -k");
> > >
> > > + /* Get name of temporary socket for unix datagram client */
> > > + if ((family == AF_UNIX) && uflag && !lflag) {
> > > + if(pflag) {
> > > + unix_dg_tmp_socket = pflag;
> > > + } else {
> > > + if((unix_dg_tmp_socket = (char *)malloc(19)) == NULL)
> > > + errx(1, "not enough memory");
> >
> > Style nit: space between if and (.
>
> OK. My diff was bad about that, so I fixed the other cases as well.
>
> > > + strlcpy(unix_dg_tmp_socket, "/tmp/nc.XXXXXXXXXX", 19);
> > > + mktemp(unix_dg_tmp_socket);
> >
> > What if this fails?
>
> You're right, a failure of mktemp should definitely be checked.
>
> > > + }
> > > + }
> > > +
> > > /* Initialize addrinfo structure. */
> > > if (family != AF_UNIX) {
> > > memset(&hints, 0, sizeof(struct addrinfo));
> > > @@ -307,8 +319,12 @@ main(int argc, char *argv[])
> > > int connfd;
> > > ret = 0;
> > >
> > > - if (family == AF_UNIX)
> > > - s = unix_listen(host);
> > > + if (family == AF_UNIX) {
> > > + if(uflag)
> > > + s = unix_bind(host);
> > > + else
> > > + s = unix_listen(host);
> > > + }
> > >
> > > /* Allow only one connection at a time, but stay alive. */
> > > for (;;) {
> > > @@ -337,17 +353,19 @@ main(int argc, char *argv[])
> > > if (rv < 0)
> > > err(1, "connect");
> > >
> > > - connfd = s;
> > > + readwrite(s);
> > > } else {
> > > len = sizeof(cliaddr);
> > > connfd = accept(s, (struct sockaddr *)&cliaddr,
> > > &len);
> > > + readwrite(connfd);
> > > + close(connfd);
> > > }
> > >
> > > - readwrite(connfd);
> > > - close(connfd);
> > > if (family != AF_UNIX)
> > > close(s);
> > > + else if (uflag)
> > > + connect(s, NULL, 0);
> >
> > Likewise.
>
> Correct, this should be checked as well.
>
> > >
> > > if (!kflag)
> > > break;
> > > @@ -361,6 +379,8 @@ main(int argc, char *argv[])
> > > } else
> > > ret = 1;
> > >
> > > + if(uflag)
> > > + unlink(unix_dg_tmp_socket);
> >
> > Shouldn't this have the same condition as above?
>
> The condition when the temp data socket is created is:
>
> (family == AF_UNIX) && uflag && !lflag
>
> The code for this section is (lines in new diff):
>
> if (lflag) { // line 319
> } else if (family == AF_UNIX) { // line 376
> if (uflag) // line 385
> } // line 389
>
> So this does have the same conditions as above.
Okay, good.
>
> Here's the new diff, containing the following changes from the previous
> diff:
>
> 1) Spaces between if and (
> 2) Switch from using -p to -s to specify source socket when using
> -U and -u.
> 3) Error checking for mktemp and connect.
> 4) man page changes
>
> Jeremy
>
> Index: atomicio.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/nc/atomicio.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 atomicio.c
> --- atomicio.c 7 Sep 2007 14:50:44 -0000 1.9
> +++ atomicio.c 6 Jan 2011 21:48:04 -0000
> @@ -53,7 +53,7 @@ atomicio(ssize_t (*f) (int, void *, size
> case -1:
> if (errno == EINTR)
> continue;
> - if (errno == EAGAIN) {
> + if ((errno == EAGAIN) || (errno == ENOBUFS)) {
> (void)poll(&pfd, 1, -1);
> continue;
> }
> Index: nc.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/nc/nc.1,v
> retrieving revision 1.55
> diff -u -p -r1.55 nc.1
> --- nc.1 25 Jul 2010 07:51:39 -0000 1.55
> +++ nc.1 7 Jan 2011 16:39:07 -0000
> @@ -155,6 +155,10 @@ assigns them.
> Enables the RFC 2385 TCP MD5 signature option.
> .It Fl s Ar source_ip_address
> Specifies the IP of the interface which is used to send the packets.
> +For
> +.Ux Ns -domain
> +datagram sockets, specifies the local temporary socket file
> +to create and use so that datagrams can be received.
> It is an error to use this option in conjunction with the
> .Fl l
> option.
> @@ -179,6 +183,9 @@ Specifies to use
> sockets.
> .It Fl u
> Use UDP instead of the default option of TCP.
> +For
> +.Ux Ns -domain
> +sockets, use a datagram socket instead of a stream socket.
> .It Fl V Ar rtable
> Set the routing table to be used.
> The default is 0.
> Index: netcat.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/nc/netcat.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 netcat.c
> --- netcat.c 3 Jul 2010 04:44:51 -0000 1.98
> +++ netcat.c 7 Jan 2011 16:41:59 -0000
> @@ -89,6 +89,7 @@ u_int rtableid;
> int timeout = -1;
> int family = AF_UNSPEC;
> char *portlist[PORT_MAX+1];
> +char *unix_dg_tmp_socket;
>
> void atelnet(int, unsigned char *, unsigned int);
> void build_ports(char *);
> @@ -99,6 +100,7 @@ int remote_connect(const char *, const c
> int socks_connect(const char *, const char *, struct addrinfo,
> const char *, const char *, struct addrinfo, int, const char *);
> int udptest(int);
> +int unix_bind(char *);
> int unix_connect(char *);
> int unix_listen(char *);
> void set_common_sockopts(int);
> @@ -241,8 +243,6 @@ main(int argc, char *argv[])
>
> /* Cruft to make sure options are clean, and used properly. */
> if (argv[0] && !argv[1] && family == AF_UNIX) {
> - if (uflag)
> - errx(1, "cannot use -u and -U");
> host = argv[0];
> uport = NULL;
> } else if (argv[0] && !argv[1]) {
> @@ -265,6 +265,19 @@ main(int argc, char *argv[])
> if (!lflag && kflag)
> errx(1, "must use -l with -k");
>
> + /* Get name of temporary socket for unix datagram client */
> + if ((family == AF_UNIX) && uflag && !lflag) {
> + if (sflag) {
> + unix_dg_tmp_socket = sflag;
> + } else {
> + if ((unix_dg_tmp_socket = (char *)malloc(19)) == NULL)
> + errx(1, "not enough memory");
> + strlcpy(unix_dg_tmp_socket, "/tmp/nc.XXXXXXXXXX", 19);
> + if (mktemp(unix_dg_tmp_socket) == NULL)
> + err(1, "mktemp");
> + }
> + }
> +
> /* Initialize addrinfo structure. */
> if (family != AF_UNIX) {
> memset(&hints, 0, sizeof(struct addrinfo));
> @@ -307,8 +320,12 @@ main(int argc, char *argv[])
> int connfd;
> ret = 0;
>
> - if (family == AF_UNIX)
> - s = unix_listen(host);
> + if (family == AF_UNIX) {
> + if (uflag)
> + s = unix_bind(host);
> + else
> + s = unix_listen(host);
> + }
>
> /* Allow only one connection at a time, but stay alive. */
> for (;;) {
> @@ -337,17 +354,21 @@ main(int argc, char *argv[])
> if (rv < 0)
> err(1, "connect");
>
> - connfd = s;
> + readwrite(s);
> } else {
> len = sizeof(cliaddr);
> connfd = accept(s, (struct sockaddr *)&cliaddr,
> &len);
> + readwrite(connfd);
> + close(connfd);
> }
>
> - readwrite(connfd);
> - close(connfd);
> if (family != AF_UNIX)
> close(s);
> + else if (uflag) {
> + if (connect(s, NULL, 0) < 0)
> + err(1, "connect");
> + }
>
> if (!kflag)
> break;
> @@ -361,6 +382,8 @@ main(int argc, char *argv[])
> } else
> ret = 1;
>
> + if (uflag)
> + unlink(unix_dg_tmp_socket);
> exit(ret);
>
> } else {
> @@ -421,18 +444,19 @@ main(int argc, char *argv[])
> }
>
> /*
> - * unix_connect()
> - * Returns a socket connected to a local unix socket. Returns -1 on failure.
> + * unix_bind()
> + * Returns a unix socket bound to the given path
> */
> int
> -unix_connect(char *path)
> +unix_bind(char *path)
> {
> struct sockaddr_un sun;
> int s;
>
> - if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
> + /* Create unix domain socket. */
> + if ((s = socket(AF_UNIX, uflag ? SOCK_DGRAM : SOCK_STREAM,
> + 0)) < 0)
> return (-1);
> - (void)fcntl(s, F_SETFD, 1);
>
> memset(&sun, 0, sizeof(struct sockaddr_un));
> sun.sun_family = AF_UNIX;
> @@ -443,27 +467,32 @@ unix_connect(char *path)
> errno = ENAMETOOLONG;
> return (-1);
> }
> - if (connect(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) < 0) {
> +
> + if (bind(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) < 0) {
> close(s);
> return (-1);
> }
> return (s);
> -
> }
>
> /*
> - * unix_listen()
> - * Create a unix domain socket, and listen on it.
> + * unix_connect()
> + * Returns a socket connected to a local unix socket. Returns -1 on failure.
> */
> int
> -unix_listen(char *path)
> +unix_connect(char *path)
> {
> struct sockaddr_un sun;
> int s;
>
> - /* Create unix domain socket. */
> - if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
> - return (-1);
> + if (uflag) {
> + if ((s = unix_bind(unix_dg_tmp_socket)) < 0)
> + return (-1);
> + } else {
> + if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
> + return (-1);
> + }
> + (void)fcntl(s, F_SETFD, 1);
>
> memset(&sun, 0, sizeof(struct sockaddr_un));
> sun.sun_family = AF_UNIX;
> @@ -474,11 +503,24 @@ unix_listen(char *path)
> errno = ENAMETOOLONG;
> return (-1);
> }
> -
> - if (bind(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) < 0) {
> + if (connect(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) < 0) {
> close(s);
> return (-1);
> }
> + return (s);
> +
> +}
> +
> +/*
> + * unix_listen()
> + * Create a unix domain socket, and listen on it.
> + */
> +int
> +unix_listen(char *path)
> +{
> + int s;
> + if ((s = unix_bind(path)) < 0)
> + return (-1);
>
> if (listen(s, 5) < 0) {
> close(s);
