fetch: Fix infinite loop on display progress
authorJohn Marino <draco@marino.st>
Thu, 1 Nov 2012 21:21:52 +0000 (22:21 +0100)
committerJohn Marino <draco@marino.st>
Thu, 1 Nov 2012 21:56:19 +0000 (22:56 +0100)
As reported by FreeBSD PR bin/153240, fetch can loop forever if it is
interrupted by a signal at just the right time.

Verbatim from FreeBSD SVN 230307 (18 JAN 2012):
Fix two issues related to the use of SIGINFO in fetch(1) to display
progress information.  The first is that fetch_read() (used in the HTTP
code but not the FTP code) can enter an infinite loop if it has previously
been interrupted by a signal.  The second is that when it is interrupted,
fetch_read() will discard any data it may have read up to that point.
Luckily, both bugs are extremely timing-sensitive and therefore difficult

to trigger.

lib/libfetch/common.c
lib/libfetch/common.h
lib/libfetch/http.c
usr.bin/fetch/fetch.c

index 86b92a2..1f5c8c6 100644 (file)
@@ -403,6 +403,33 @@ fetch_ssl_read(SSL *ssl, char *buf, size_t len)
 }
 #endif
 
+/*
+ * Cache some data that was read from a socket but cannot be immediately
+ * returned because of an interrupted system call.
+ */
+static int
+fetch_cache_data(conn_t *conn, char *src, size_t nbytes)
+{
+       char *tmp;
+
+       if (conn->cache.size < nbytes) {
+               tmp = realloc(conn->cache.buf, nbytes);
+               if (tmp == NULL) {
+                       fetch_syserr();
+                       return (-1);
+               }
+               conn->cache.buf = tmp;
+               conn->cache.size = nbytes;
+       }
+
+       memcpy(conn->cache.buf, src, nbytes);
+       conn->cache.len = nbytes;
+       conn->cache.pos = 0;
+
+       return (0);
+}
+
+
 static ssize_t
 fetch_socket_read(int sd, char *buf, size_t len)
 {
@@ -428,6 +455,7 @@ fetch_read(conn_t *conn, char *buf, size_t len)
        fd_set readfds;
        ssize_t rlen, total;
        int r;
+       char *start;
 
        if (fetchTimeout) {
                FD_ZERO(&readfds);
@@ -436,6 +464,24 @@ fetch_read(conn_t *conn, char *buf, size_t len)
        }
 
        total = 0;
+       start = buf;
+
+       if (conn->cache.len > 0) {
+               /*
+                * The last invocation of fetch_read was interrupted by a
+                * signal after some data had been read from the socket. Copy
+                * the cached data into the supplied buffer before trying to
+                * read from the socket again.
+                */
+               total = (conn->cache.len < len) ? conn->cache.len : len;
+               memcpy(buf, conn->cache.buf, total);
+
+               conn->cache.len -= total;
+               conn->cache.pos += total;
+               len -= total;
+               buf += total;
+       }
+
        while (len > 0) {
                /*
                 * The socket is non-blocking.  Instead of the canonical
@@ -471,6 +517,8 @@ fetch_read(conn_t *conn, char *buf, size_t len)
                        total += rlen;
                        continue;
                } else if (rlen == FETCH_READ_ERROR) {
+                       if (errno == EINTR)
+                               fetch_cache_data(conn, start, total);
                        return (-1);
                }
                // assert(rlen == FETCH_READ_WAIT);
@@ -491,8 +539,12 @@ fetch_read(conn_t *conn, char *buf, size_t len)
                        errno = 0;
                        r = select(conn->sd + 1, &readfds, NULL, NULL, &delta);
                        if (r == -1) {
-                               if (errno == EINTR && fetchRestartCalls)
-                                       continue;
+                               if (errno == EINTR) {
+                                       if (fetchRestartCalls)
+                                               continue;
+                                       /* Save anything that was read. */
+                                       fetch_cache_data(conn, start, total);
+                               }
                                fetch_syserr();
                                return (-1);
                        }
@@ -676,6 +728,7 @@ fetch_close(conn_t *conn)
        if (--conn->ref > 0)
                return (0);
        ret = close(conn->sd);
+       free(conn->cache.buf);
        free(conn->buf);
        free(conn);
        return (ret);
index 1ba13da..9af59d8 100644 (file)
@@ -26,7 +26,6 @@
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
  * $FreeBSD: src/lib/libfetch/common.h,v 1.30 2007/12/18 11:03:07 des Exp $
- * $DragonFly: src/lib/libfetch/common.h,v 1.3 2007/08/05 21:48:12 swildner Exp $
  */
 
 #ifndef _COMMON_H_INCLUDED
@@ -53,6 +52,13 @@ struct fetchconn {
        size_t           bufsize;       /* buffer size */
        size_t           buflen;        /* length of buffer contents */
        int              err;           /* last protocol reply code */
+       struct {                        /* data cached after an interrupted
+                                          read */
+               char    *buf;
+               size_t   size;
+               size_t   pos;
+               size_t   len;
+       } cache;
 #ifdef WITH_SSL
        SSL             *ssl;           /* SSL handle */
        SSL_CTX         *ssl_ctx;       /* SSL context */
index a174cd7..30ec92d 100644 (file)
@@ -197,6 +197,8 @@ http_growbuf(struct httpio *io, size_t len)
 static int
 http_fillbuf(struct httpio *io, size_t len)
 {
+       ssize_t nbytes;
+
        if (io->error)
                return (-1);
        if (io->eof)
@@ -205,10 +207,11 @@ http_fillbuf(struct httpio *io, size_t len)
        if (io->chunked == 0) {
                if (http_growbuf(io, len) == -1)
                        return (-1);
-               if ((io->buflen = fetch_read(io->conn, io->buf, len)) == -1) {
-                       io->error = 1;
+               if ((nbytes = fetch_read(io->conn, io->buf, len)) == -1) {
+                       io->error = errno;
                        return (-1);
                }
+               io->buflen = nbytes;
                io->bufpos = 0;
                return (io->buflen);
        }
@@ -228,10 +231,11 @@ http_fillbuf(struct httpio *io, size_t len)
                len = io->chunksize;
        if (http_growbuf(io, len) == -1)
                return (-1);
-       if ((io->buflen = fetch_read(io->conn, io->buf, len)) == -1) {
-               io->error = 1;
+       if ((nbytes = fetch_read(io->conn, io->buf, len)) == -1) {
+               io->error = errno;
                return (-1);
        }
+       io->buflen = nbytes;
        io->chunksize -= io->buflen;
 
        if (io->chunksize == 0) {
@@ -273,8 +277,11 @@ http_readfn(void *v, char *buf, int len)
                io->bufpos += l;
        }
 
-       if (!pos && io->error)
+       if (!pos && io->error) {
+               if (io->error == EINTR)
+                       io->error = 0;
                return (-1);
+       }
        return (pos);
 }
 
index a3f32b0..303241b 100644 (file)
@@ -26,7 +26,6 @@
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
  * $FreeBSD: src/usr.bin/fetch/fetch.c,v 1.84 2009/01/17 13:34:56 des Exp $
- * $DragonFly: src/usr.bin/fetch/fetch.c,v 1.8 2007/08/05 21:48:12 swildner Exp $
  */
 
 #include <sys/param.h>
@@ -317,7 +316,7 @@ fetch(char *URL, const char *path)
        struct stat sb, nsb;
        struct xferstat xs;
        FILE *f, *of;
-       size_t size, wr;
+       size_t size, readcnt, wr;
        off_t count;
        char flags[8];
        const char *slash;
@@ -621,21 +620,26 @@ fetch(char *URL, const char *path)
                        stat_end(&xs);
                        siginfo = 0;
                }
-               if ((size = fread(buf, 1, size, f)) == 0) {
+
+               if (size == 0)
+                       break;
+
+               if ((readcnt = fread(buf, 1, size, f)) < size) {
                        if (ferror(f) && errno == EINTR && !sigint)
                                clearerr(f);
-                       else
+                       else if (readcnt == 0)
                                break;
                }
-               stat_update(&xs, count += size);
-               for (ptr = buf; size > 0; ptr += wr, size -= wr)
-                       if ((wr = fwrite(ptr, 1, size, of)) < size) {
+
+               stat_update(&xs, count += readcnt);
+               for (ptr = buf; readcnt > 0; ptr += wr, readcnt -= wr)
+                       if ((wr = fwrite(ptr, 1, readcnt, of)) < readcnt) {
                                if (ferror(of) && errno == EINTR && !sigint)
                                        clearerr(of);
                                else
                                        break;
                        }
-               if (size != 0)
+               if (readcnt != 0)
                        break;
        }
        if (!sigalrm)