statschannel doesn't handle multiple reads correctly

if an incoming HTTP request is incomplete, but nothing else is clearly
wrong with it, the stats channel continues reading to see if there's
more coming.  the buffer length was not being processed correctly in
this case.  also, the server state was not reset correctly when the
request was complete, so that subsequent requests could be appended to
the first buffer instead of being treated as new.

in addition fixing the above problems, this commit also increases the
size of the httpd request buffer from 1024 to 4096, because some
browsers send a lot of headers.

diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c
index b71d323823ba3af661dbe694809c9cfabee54fb2..c9f78ef33196a6d64993c32e1cbce05d51459431 100644 (file)
--- a/lib/isc/httpd.c
+++ b/lib/isc/httpd.c
@@ -38,7 +38,7 @@
                }                              \
        } while (0)
 
-#define HTTP_RECVLEN    1024
+#define HTTP_RECVLEN    4096
 #define HTTP_SENDGROW   1024
 #define HTTP_SEND_MAXLEN 10240
 
@@ -117,7 +117,6 @@ struct isc_httpd {
         * the backing store.
         * The third buffer is compbuffer, managed by us, that contains the
         * compressed HTTP data, if compression is used.
-        *
         */
        isc_buffer_t headerbuffer;
        isc_buffer_t compbuffer;
@@ -172,7 +171,7 @@ static isc_result_t
 httpd_response(isc_httpd_t *);
 
 static isc_result_t
-process_request(isc_httpd_t *, isc_region_t *);
+process_request(isc_httpd_t *, isc_region_t *, size_t *);
 static isc_result_t
 grow_headerspace(isc_httpd_t *);
 
@@ -196,7 +195,7 @@ free_buffer(isc_mem_t *mctx, isc_buffer_t *buffer) {
        isc_region_t r;
 
        isc_buffer_region(buffer, &r);
-       if (r.length > 0) {
+       if (r.base != NULL) {
                isc_mem_put(mctx, r.base, r.length);
        }
 }
@@ -393,13 +392,20 @@ have_header(isc_httpd_t *httpd, const char *header, const char *value,
 }
 
 static isc_result_t
-process_request(isc_httpd_t *httpd, isc_region_t *region) {
-       char *s = NULL, *p = NULL;
+process_request(isc_httpd_t *httpd, isc_region_t *region, size_t *buflen) {
+       char *s = NULL, *p = NULL, *urlend = NULL;
+       size_t limit = sizeof(httpd->recvbuf) - httpd->recvlen - 1;
+       size_t len = region->length;
        int delim;
 
-       memmove(httpd->recvbuf + httpd->recvlen, region->base, region->length);
-       httpd->recvlen += region->length;
+       if (len > limit) {
+               len = limit;
+       }
+
+       memmove(httpd->recvbuf + httpd->recvlen, region->base, len);
+       httpd->recvlen += len;
        httpd->recvbuf[httpd->recvlen] = 0;
+       *buflen = httpd->recvlen;
        httpd->headers = NULL;
 
        /*
@@ -417,7 +423,7 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) {
        }
 
        /*
-        * NUL terminate request at the blank line.
+        * NULL terminate the request at the blank line.
         */
        s[delim] = 0;
 
@@ -454,13 +460,13 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) {
        if (!BUFLENOK(s)) {
                return (ISC_R_NOMEMORY);
        }
-       *s = 0;
+       urlend = s;
 
        /*
         * Make the URL relative.
         */
-       if ((strncmp(p, "http:/", 6) == 0) || (strncmp(p, "https:/", 7) == 0)) {
-               /* Skip first / */
+       if (strncmp(p, "http://", 7) == 0 || strncmp(p, "https://", 8) == 0) {
+               /* Skip first '/' */
                while (*p != '/' && *p != 0) {
                        p++;
                }
@@ -468,7 +474,7 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) {
                        return (ISC_R_RANGE);
                }
                p++;
-               /* Skip second / */
+               /* Skip second '/' */
                while (*p != '/' && *p != 0) {
                        p++;
                }
@@ -476,7 +482,7 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) {
                        return (ISC_R_RANGE);
                }
                p++;
-               /* Find third / */
+               /* Find third '/' */
                while (*p != '/' && *p != 0) {
                        p++;
                }
@@ -491,7 +497,7 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) {
        s = p;
 
        /*
-        * Now, see if there is a ? mark in the URL.  If so, this is
+        * Now, see if there is a question mark in the URL.  If so, this is
         * part of the query string, and we will split it from the URL.
         */
        httpd->querystring = strchr(httpd->url, '?');
@@ -566,6 +572,16 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) {
                return (ISC_R_RANGE);
        }
 
+       /*
+        * Looks like a a valid request, so now we know we won't have
+        * to process this buffer again. We can NULL-terminate the
+        * URL for the caller's benefit, and set recvlen to 0 so
+        * the next read will overwrite this one instead of appending
+        * to the buffer.
+        */
+       *urlend = 0;
+       httpd->recvlen = 0;
+
        return (ISC_R_SUCCESS);
 }
 
@@ -596,24 +612,24 @@ httpd_reset(void *arg) {
        isc_buffer_clear(&httpd->headerbuffer);
        isc_buffer_clear(&httpd->compbuffer);
        isc_buffer_invalidate(&httpd->bodybuffer);
-
-       httpdmgr_detach(&httpdmgr);
 }
 
 static void
 httpd_put(void *arg) {
        isc_httpd_t *httpd = (isc_httpd_t *)arg;
-       isc_httpdmgr_t *httpdmgr = NULL;
+       isc_httpdmgr_t *mgr = NULL;
 
        REQUIRE(VALID_HTTPD(httpd));
 
-       httpdmgr = httpd->mgr;
-       REQUIRE(VALID_HTTPDMGR(httpdmgr));
+       mgr = httpd->mgr;
+       REQUIRE(VALID_HTTPDMGR(mgr));
 
        httpd->magic = 0;
+       httpd->mgr = NULL;
 
-       free_buffer(httpdmgr->mctx, &httpd->headerbuffer);
-       free_buffer(httpdmgr->mctx, &httpd->compbuffer);
+       free_buffer(mgr->mctx, &httpd->headerbuffer);
+       free_buffer(mgr->mctx, &httpd->compbuffer);
+       httpdmgr_detach(&mgr);
 
 #if ENABLE_AFL
        if (finishhook != NULL) {
@@ -633,6 +649,7 @@ new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) {
        if (httpd == NULL) {
                httpd = isc_nmhandle_getextra(handle);
                *httpd = (isc_httpd_t){ .handle = NULL };
+               httpdmgr_attach(httpdmgr, &httpd->mgr);
        }
 
        if (httpd->handle == NULL) {
@@ -642,8 +659,6 @@ new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) {
                INSIST(httpd->handle == handle);
        }
 
-       httpdmgr_attach(httpdmgr, &httpd->mgr);
-
        /*
         * Initialize the buffer for our headers.
         */
@@ -749,22 +764,15 @@ render_500(const char *url, isc_httpdurl_t *urlinfo, const char *querystring,
 /*%<
  * Reallocates compbuffer to size, does nothing if compbuffer is already
  * larger than size.
- *
- * Requires:
- *\li  httpd a valid isc_httpd_t object
- *
- * Returns:
- *\li  #ISC_R_SUCCESS          -- all is well.
- *\li  #ISC_R_NOMEMORY         -- not enough memory to extend buffer
  */
-static isc_result_t
+static void
 alloc_compspace(isc_httpd_t *httpd, unsigned int size) {
-       char *newspace;
+       char *newspace = NULL;
        isc_region_t r;
 
        isc_buffer_region(&httpd->compbuffer, &r);
        if (size < r.length) {
-               return (ISC_R_SUCCESS);
+               return;
        }
 
        newspace = isc_mem_get(httpd->mgr->mctx, size);
@@ -773,8 +781,6 @@ alloc_compspace(isc_httpd_t *httpd, unsigned int size) {
        if (r.base != NULL) {
                isc_mem_put(httpd->mgr->mctx, r.base, r.length);
        }
-
-       return (ISC_R_SUCCESS);
 }
 
 /*%<
@@ -794,15 +800,11 @@ static isc_result_t
 httpd_compress(isc_httpd_t *httpd) {
        z_stream zstr;
        isc_region_t r;
-       isc_result_t result;
        int ret;
        int inputlen;
 
        inputlen = isc_buffer_usedlength(&httpd->bodybuffer);
-       result = alloc_compspace(httpd, inputlen);
-       if (result != ISC_R_SUCCESS) {
-               return (result);
-       }
+       alloc_compspace(httpd, inputlen);
        isc_buffer_region(&httpd->compbuffer, &r);
 
        /*
@@ -842,6 +844,7 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult,
        isc_region_t r;
        bool is_compressed = false;
        char datebuf[ISC_FORMATHTTPTIMESTAMP_SIZE];
+       size_t buflen = 0;
 
        httpd = isc_nmhandle_getdata(handle);
 
@@ -851,10 +854,10 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult,
                goto cleanup_readhandle;
        }
 
-       result = process_request(httpd, region);
+       result = process_request(httpd, region, &buflen);
        if (result == ISC_R_NOTFOUND) {
-               if (httpd->recvlen < HTTP_RECVLEN - 1) {
-                       /* don't unref, continue reading */
+               if (buflen < HTTP_RECVLEN - 1) {
+                       /* don't unref, keep reading */
                        return;
                }
                goto cleanup_readhandle;
@@ -960,7 +963,6 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult,
        isc_nmhandle_attach(handle, &httpd->sendhandle);
        isc_nm_send(handle, &r, httpd_senddone, httpd);
 
-       return;
 cleanup_readhandle:
        isc_nmhandle_detach(&httpd->readhandle);
 }