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.
2025-08-31 06:25:31 +00:00 · 2021-10-22 15:58:46 -07:00
parent 76375797b5
commit cbf8c2e019
1 changed files with 47 additions and 45 deletions
--- 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);
 }