Merge branch '2973-http-buffer-fix' into 'main'

statschannel doesn't handle multiple reads correctly

Closes #2973

See merge request isc-projects/bind9!5530

CHANGES

@@ -1,3 +1,7 @@
+5755.	[bug]		The statistics channel wasn't correctly handling
+			multiple HTTP requests, or pipelined or truncated
+			requests. [GL #2973]
+
 5754.	[bug]		"tls" statements may omit "key-file" and "cert-file",
 			but if either one is specified, then both must be.
 			[GL #2986]

bin/tests/system/conf.sh.in

@@ -112,6 +112,9 @@ SHELL=@SHELL@
 # CURL will be empty if no program was found by configure
 CURL=@CURL@
 
+# NC will be empty if no program was found by configure
+NC=@NC@
+
 # XMLLINT will be empty if no program was found by configure
 XMLLINT=@XMLLINT@
 

bin/tests/system/statistics/tests.sh

@@ -175,8 +175,12 @@ echo_i "checking bind9.xsl vs xml ($n)"
 if $FEATURETEST --have-libxml2 && [ -x "${CURL}" ] && [ -x "${XSLTPROC}" ]  ; then
     $DIGCMD +notcp +recurse @10.53.0.3 soa . > dig.out.test$n.1 2>&1
     $DIGCMD +notcp +recurse @10.53.0.3 soa example > dig.out.test$n.2 2>&1
-    ${CURL} http://10.53.0.3:${EXTRAPORT1}/xml/v3 > curl.out.${n}.xml 2>/dev/null || ret=1
-    ${CURL} http://10.53.0.3:${EXTRAPORT1}/bind9.xsl > curl.out.${n}.xsl 2>/dev/null || ret=1
+    # check multiple requests over the same socket
+    time1=$($PERL -e 'print time(), "\n";')
+    ${CURL} --http1.1 -o curl.out.${n}.xml http://10.53.0.3:${EXTRAPORT1}/xml/v3 \
+	    -o curl.out.${n}.xsl http://10.53.0.3:${EXTRAPORT1}/bind9.xsl 2>/dev/null || ret=1
+    time2=$($PERL -e 'print time(), "\n";')
+    test $((time2 - time1)) -lt 5 || ret=1
     ${DIFF} ${TOP_SRCDIR}/bin/named/bind9.xsl curl.out.${n}.xsl || ret=1
     ${XSLTPROC} curl.out.${n}.xsl - < curl.out.${n}.xml > xsltproc.out.${n} 2>/dev/null || ret=1
     cp curl.out.${n}.xml stats.xml.out || ret=1

bin/tests/system/statschannel/clean.sh

@@ -28,3 +28,5 @@ rm -f xml.*mem json.*mem
 rm -f xml.*stats json.*stats
 rm -f zones zones.out.* zones.json.* zones.xml.* zones.expect.*
 rm -rf ./__pycache__
+rm -f nc.out*
+rm -f send.in* send.out*

bin/tests/system/statschannel/send64k.pl

@@ -0,0 +1,44 @@
+#!/usr/bin/perl
+#
+# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, you can obtain one at https://mozilla.org/MPL/2.0/.
+#
+# See the COPYRIGHT file distributed with this work for additional
+# information regarding copyright ownership.
+
+#
+# Send a file to a given address and port using TCP.  Used for
+# configuring the test server in ans.pl.
+#
+
+use IO::File;
+use IO::Socket;
+
+@ARGV == 2 or die "usage: send.pl host port\n";
+
+my $host = shift @ARGV;
+my $port = shift @ARGV;
+
+my $sock = IO::Socket::INET->new(PeerAddr => $host, PeerPort => $port,
+				 Proto => "tcp",) or die "$!";
+#send the file
+while ($n = read(STDIN, $buf, 64000)) {
+	$sock->syswrite($buf, $n);
+}
+
+#get the response with with a 15 second timeout
+my $rin;
+my $rout;
+my $n;
+do {
+        $rin = '';
+        vec($rin, fileno($sock), 1) = 1;
+	$n = select($rout = $rin, undef, undef, 15);
+	$n = $sock->sysread($buf, 64000) if ($n > 0);
+	print STDOUT $buf if ($n > 0);
+} while ($n > 0);
+
+$sock->close;

bin/tests/system/statschannel/tests.sh

@@ -374,5 +374,53 @@ if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 n=`expr $n + 1`
 
+if [ -x "${NC}" ] ; then
+    echo_i "Check HTTP/1.1 pipelined requests are handled ($n)"
+    ret=0
+    ${NC} 10.53.0.3 ${EXTRAPORT1} << EOF > nc.out$n || ret=1
+GET /xml/v3/status HTTP/1.1
+Host: 10.53.0.3:${EXTRAPORT1}
+
+GET /xml/v3/status HTTP/1.1
+Host: 10.53.0.3:${EXTRAPORT1}
+Connection: close
+
+EOF
+    lines=$(grep "^HTTP/1.1" nc.out$n | wc -l)
+    test $lines = 2 || ret=1
+    if [ $ret != 0 ]; then echo_i "failed"; fi
+    status=`expr $status + $ret`
+    n=`expr $n + 1`
+else
+    echo_i "skipping test as nc not found"
+fi
+
+echo_i "Check HTTP/1.1 pipelined with truncated stream ($n)"
+ret=0
+i=0
+# build input stream.
+cp /dev/null send.in$n
+while test $i -lt 500
+do
+cat >> send.in$n << EOF
+GET /xml/v3/status HTTP/1.1
+Host: 10.53.0.3
+
+EOF
+i=$((i+1))
+done
+
+# send the requests then wait for named to close the socket.
+time1=$($PERL -e 'print time(), "\n";')
+${PERL} send64k.pl 10.53.0.3 ${EXTRAPORT1} < send.in$n  > send.out$n
+time2=$($PERL -e 'print time(), "\n";')
+test $((time2 - time1)) -lt 5 || ret=1
+# we expect 91 of the 500 requests to be processed.
+lines=$(grep "^HTTP/1.1" send.out$n | wc -l)
+test $lines = 91 || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+n=`expr $n + 1`
+
 echo_i "exit status: $status"
 [ $status -eq 0 ] || exit 1

configure.ac

@@ -1256,6 +1256,13 @@ AC_CONFIG_FILES([doc/doxygen/doxygen-input-filter],
 AC_PATH_PROG(CURL, curl, curl)
 AC_SUBST(CURL)
 
+#
+# Look for nc
+#
+
+AC_PATH_PROGS(NC, nc, nc)
+AC_SUBST(NC)
+
 #
 # IDN support using libidn2
 #

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
 
@@ -87,7 +87,9 @@ struct isc_httpd {
 	 */
 	char recvbuf[HTTP_RECVLEN]; /*%< receive buffer */
 	uint32_t recvlen;	    /*%< length recv'd */
+	uint32_t consume;	    /*%< length of last command */
 	char *headers;		    /*%< set in process_request() */
+	bool truncated;
 	method_t method;
 	char *url;
 	char *querystring;
@@ -117,7 +119,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 +173,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 +197,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,14 +394,32 @@ 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;
+	bool truncated = false;
 
-	memmove(httpd->recvbuf + httpd->recvlen, region->base, region->length);
-	httpd->recvlen += region->length;
-	httpd->recvbuf[httpd->recvlen] = 0;
+	if (len > limit) {
+		len = limit;
+		truncated = true;
+	}
+
+	if (len > 0U) {
+		if (httpd->truncated) {
+			return (ISC_R_NOSPACE);
+		}
+		memmove(httpd->recvbuf + httpd->recvlen, region->base, len);
+		httpd->recvlen += len;
+		httpd->recvbuf[httpd->recvlen] = 0;
+		isc_region_consume(region, len);
+	}
+	if (truncated) {
+		httpd->truncated = true;
+	}
 	httpd->headers = NULL;
+	*buflen = httpd->recvlen;
 
 	/*
 	 * If we don't find a blank line in our buffer, return that we need
@@ -411,13 +430,17 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) {
 	if (s == NULL) {
 		s = strstr(httpd->recvbuf, "\n\n");
 		delim = 1;
-	}
-	if (s == NULL) {
-		return (ISC_R_NOTFOUND);
+		if (s == NULL) {
+			return (httpd->truncated ? ISC_R_NOSPACE
+						 : ISC_R_NOTFOUND);
+		}
+		httpd->consume = s + 2 - httpd->recvbuf;
+	} else {
+		httpd->consume = s + 4 - httpd->recvbuf;
 	}
 
 	/*
-	 * NUL terminate request at the blank line.
+	 * NULL terminate the request at the blank line.
 	 */
 	s[delim] = 0;
 
@@ -454,13 +477,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 +491,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 +499,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 +514,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 +589,15 @@ 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;
+
 	return (ISC_R_SUCCESS);
 }
 
@@ -586,6 +618,8 @@ httpd_reset(void *arg) {
 
 	httpd->recvbuf[0] = 0;
 	httpd->recvlen = 0;
+	httpd->consume = 0;
+	httpd->truncated = false;
 	httpd->headers = NULL;
 	httpd->method = METHOD_UNKNOWN;
 	httpd->url = NULL;
@@ -596,24 +630,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 +667,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 +677,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 +782,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 +799,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 +818,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 +862,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 +872,22 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult,
 		goto cleanup_readhandle;
 	}
 
-	result = process_request(httpd, region);
+	result = process_request(
+		httpd, region == NULL ? &(isc_region_t){ NULL, 0 } : region,
+		&buflen);
 	if (result == ISC_R_NOTFOUND) {
-		if (httpd->recvlen < HTTP_RECVLEN - 1) {
-			/* don't unref, continue reading */
+		if (buflen < HTTP_RECVLEN - 1) {
+			if (region != NULL) {
+				/* don't unref, keep reading */
+				return;
+			}
+			/*
+			 * We have been called from httpd_senddone
+			 * and we need to resume reading.  Detach
+			 * readhandle before resuming.
+			 */
+			isc_nmhandle_detach(&httpd->readhandle);
+			isc_nm_resumeread(handle);
 			return;
 		}
 		goto cleanup_readhandle;
@@ -943,12 +976,25 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult,
 	 * the response headers and store the result in httpd->sendbuffer.
 	 */
 	isc_buffer_dup(mgr->mctx, &httpd->sendbuffer, &httpd->headerbuffer);
+	isc_buffer_clear(&httpd->headerbuffer);
 	isc_buffer_setautorealloc(httpd->sendbuffer, true);
 	databuffer = (is_compressed ? &httpd->compbuffer : &httpd->bodybuffer);
 	isc_buffer_usedregion(databuffer, &r);
 	result = isc_buffer_copyregion(httpd->sendbuffer, &r);
 	RUNTIME_CHECK(result == ISC_R_SUCCESS);
 
+	/* Consume the request from the recv buffer. */
+	if (httpd->consume != 0U) {
+		INSIST(httpd->consume <= httpd->recvlen);
+		if (httpd->consume < httpd->recvlen) {
+			memmove(httpd->recvbuf, httpd->recvbuf + httpd->consume,
+				httpd->recvlen - httpd->consume);
+		}
+		httpd->recvlen -= httpd->consume;
+		httpd->consume = 0;
+		httpd->recvbuf[httpd->recvlen] = 0;
+	}
+
 	/*
 	 * Determine total response size.
 	 */
@@ -960,7 +1006,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);
 }
@@ -1133,7 +1178,16 @@ httpd_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
 	}
 
 	httpd->state = RECV;
-	isc_nm_resumeread(handle);
+	if (httpd->recvlen != 0) {
+		/*
+		 * Outstanding requests still exist, start processing
+		 * them.
+		 */
+		isc_nmhandle_attach(handle, &httpd->readhandle);
+		httpd_request(handle, ISC_R_SUCCESS, NULL, httpd->mgr);
+	} else if (!httpd->truncated) {
+		isc_nm_resumeread(handle);
+	}
 }
 
 isc_result_t

util/copyrights

@@ -765,6 +765,7 @@
 ./bin/tests/system/statschannel/helper.py	PYTHON	2020,2021
 ./bin/tests/system/statschannel/mem-xml.pl	PERL	2017,2018,2019,2020,2021
 ./bin/tests/system/statschannel/ns2/sign.sh	SH	2019,2020,2021
+./bin/tests/system/statschannel/send64k.pl	PERL	2021
 ./bin/tests/system/statschannel/server-json.pl	PERL	2015,2016,2017,2018,2019,2020,2021
 ./bin/tests/system/statschannel/server-xml.pl	PERL	2015,2016,2017,2018,2019,2020,2021
 ./bin/tests/system/statschannel/setup.sh	SH	2018,2019,2020,2021