Ted Unangst wrote:
> On Mon, Jun 09, 2014 at 21:54, Theo de Raadt wrote:
>>> > A better patch is probably the following which also increases the size
>>> > of the buffer to at least 64k:
>>>
>>> Agreed.
>>
>> One thing to be aware of. That function is syncronous. It will read
>> as much as it can get, then it will do an "atomic" write operation to
>> flush the buffer out the other way.
>>
>> If you have substantially different speeds, this can be a substantial
>> 'buffer bloat'.
>>
>> Since it is handling a session in both directions... expect to see
>> some substantial jaggies.
>
> Having convinced me this a problem (it's already in the code, but 64k
> buffers will make it worse), I scaled back to 16k.
>
> Now soliciting diffs to change readwrite to a loop with two buffers
> that poll()s in all four directions. :)
The attached code is based on a stripped down I/O buffer that I wrote as a
hobby project.
I'm not a big user of netcat so I'm not sure it'll work in all kinds of
situations. I don't want to spend a lot of time on it either but I hope the
attached code is useful as is, or may possibly inspire other people to come up
with something better. e.g., I tried adding some error handling but I'm not
sure I did it right. Another thing bothering me is the atelnet function. The
way I see it is that netcat sets up two communication channels, one for
incoming traffic on a socket to stdout, another one for traffic from stdin
going out of that socket. atelnet seems to short-circuit that which feels
wrong. (I suppose atelnet can't be removed as people probably rely on it)
First I'll shortly try to explain the idea behind the buffer (and a single
communication channel).
Assume data traveling from a file descriptor (fd1) to the buffer, and from the
buffer to another file descriptor (fd2).
initial buffer:
|--------------|
<----size---->
^
cur (current position in the buffer)
start with fd1 enabled and fd2 disabled.
1) a read from fd1 writes into the buffer:
|WWWWWWWWWWWW--|
<----size---->
<---used--->
^
cur (isn't used in this case)
switch off fd1 and enable fd2.
2) a read from buffer (cur position) writes to fd2:
(cur position gets updated by amount of data written to fd2)
|RRRRWWWWWWWW--|
<----size---->
<-used->
^
cur
repeat this step until all data has been written to fd2:
|RRRRRRRRRRRR--|
<----size---->
^
cur
3) all data has been transferred, reset buffer, disable fd2 and enable fd1:
|--------------|
<----size---->
^
cur
4) repeat step 1, 2 and 3.
The patch itself:
Index: netcat.c
===================================================================
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.121
diff -u -p -u -r1.121 netcat.c
--- netcat.c 10 Jun 2014 16:35:42 -0000 1.121
+++ netcat.c 27 Jun 2014 12:42:18 -0000
@@ -732,60 +732,141 @@ local_listen(char *host, char *port, str
void
readwrite(int nfd)
{
- struct pollfd pfd[2];
- unsigned char buf[16 * 1024];
- int n, wfd = fileno(stdin);
- int lfd = fileno(stdout);
- int plen;
-
- plen = sizeof(buf);
+ static struct {
+ #define _BUFFER_SIZE (16*1024)
+ size_t size;
+ size_t used;
+ unsigned char *cur;
+ unsigned char data[_BUFFER_SIZE];
+ } buf[2] =
+ {
+ { _BUFFER_SIZE, 0, buf[0].data },
+ { _BUFFER_SIZE, 0, buf[1].data },
+ };
+ struct pollfd pfd[4];
+ int npfd;
+ ssize_t n;
/* Setup Network FD */
pfd[0].fd = nfd;
pfd[0].events = POLLIN;
+ pfd[1].fd = -1;
+ pfd[1].events = POLLOUT;
- /* Set up STDIN FD. */
- pfd[1].fd = wfd;
- pfd[1].events = POLLIN;
+ /* Set up STDIN/STDOUT FD. */
+ pfd[2].fd = -1;
+ pfd[2].events = POLLOUT;
+ pfd[3].fd = STDIN_FILENO;
+ pfd[3].events = POLLIN;
- while (pfd[0].fd != -1) {
+ while (pfd[0].fd != -1 || pfd[1].fd != -1 ||
+ pfd[2].fd != -1 || pfd[3].fd != -1) {
if (iflag)
sleep(iflag);
- if ((n = poll(pfd, 2 - dflag, timeout)) < 0) {
+ npfd = poll(pfd, 4 - dflag, timeout);
+ if (npfd < 0) {
close(nfd);
err(1, "Polling Error");
}
- if (n == 0)
+ if (npfd == 0)
return;
if (pfd[0].revents & POLLIN) {
- if ((n = read(nfd, buf, plen)) < 0)
- return;
- else if (n == 0) {
+ n = read(pfd[0].fd, buf[0].data, buf[0].size);
+ if (n == -1) {
+ if (errno != EAGAIN && errno != EINTR)
+ return;
+ } else if (n == 0) {
shutdown(nfd, SHUT_RD);
pfd[0].fd = -1;
pfd[0].events = 0;
- } else {
+ } else if (n > 0) {
+ buf[0].used = n;
if (tflag)
- atelnet(nfd, buf, n);
- if (atomicio(vwrite, lfd, buf, n) != n)
+ atelnet(nfd, buf[0].data, buf[0].used);
+ pfd[0].fd = -1;
+ pfd[2].fd = STDOUT_FILENO;
+ }
+ }
+
+ if (pfd[1].revents & POLLOUT) {
+ n = write(pfd[1].fd, buf[1].cur, buf[1].used);
+ if (n == -1) {
+ switch(errno) {
+ case EAGAIN:
+ case EINTR:
+ case ENOBUFS:
+ break;
+ case EPIPE:
+ /*close(pfd[1].fd);*/
+ pfd[1].fd = -1;
+ pfd[3].fd = -1;
+ pfd[3].events = 0;
+ break;
+ default:
return;
+ }
+ } else if (n > 0) {
+ if (n == buf[1].used) {
+ buf[1].cur = buf[1].data;
+ buf[1].used = 0;
+ pfd[1].fd = -1;
+ if (pfd[3].events & POLLIN)
+ pfd[3].fd = STDIN_FILENO;
+ } else {
+ buf[1].cur += n;
+ buf[1].used -= n;
+ }
}
}
- if (!dflag && pfd[1].revents & POLLIN) {
- if ((n = read(wfd, buf, plen)) < 0)
- return;
- else if (n == 0) {
+ if (pfd[2].revents & POLLOUT) {
+ n = write(pfd[2].fd, buf[0].cur, buf[0].used);
+ if (n == -1) {
+ switch(errno) {
+ case EAGAIN:
+ case EINTR:
+ case ENOBUFS:
+ break;
+ case EPIPE:
+ /*close(pfd[2].fd);*/
+ pfd[2].fd = -1;
+ pfd[0].fd = -1;
+ pfd[0].events = 0;
+ break;
+ default:
+ return;
+ }
+ } else if (n > 0) {
+ if (n == buf[0].used) {
+ buf[0].cur = buf[0].data;
+ buf[0].used = 0;
+ pfd[2].fd = -1;
+ if (pfd[0].events & POLLIN)
+ pfd[0].fd = nfd;
+ } else {
+ buf[0].cur += n;
+ buf[0].used -= n;
+ }
+ }
+ }
+
+ if (!dflag && pfd[3].revents & POLLIN) {
+ n = read(pfd[3].fd, buf[1].data, buf[1].size);
+ if (n == -1) {
+ if (errno != EAGAIN && errno != EINTR)
+ return;
+ } else if (n == 0) {
if (Nflag)
shutdown(nfd, SHUT_WR);
- pfd[1].fd = -1;
- pfd[1].events = 0;
- } else {
- if (atomicio(vwrite, nfd, buf, n) != n)
- return;
+ pfd[3].fd = -1;
+ pfd[3].events = 0;
+ } else if (n > 0) {
+ buf[1].used = n;
+ pfd[3].fd = -1;
+ pfd[1].fd = nfd;
}
}
}
