diff --git a/CHANGES b/CHANGES index e2e7dff829..3432aadf15 100644 --- a/CHANGES +++ b/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] diff --git a/bin/tests/system/conf.sh.in b/bin/tests/system/conf.sh.in index 06aaad39c2..ff6c6e4b67 100644 --- a/bin/tests/system/conf.sh.in +++ b/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@ diff --git a/bin/tests/system/statistics/tests.sh b/bin/tests/system/statistics/tests.sh index 4cb8008180..7e0190c604 100644 --- a/bin/tests/system/statistics/tests.sh +++ b/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 diff --git a/bin/tests/system/statschannel/clean.sh b/bin/tests/system/statschannel/clean.sh index 2e13b1a9a5..09218d5b5b 100644 --- a/bin/tests/system/statschannel/clean.sh +++ b/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* diff --git a/bin/tests/system/statschannel/send64k.pl b/bin/tests/system/statschannel/send64k.pl new file mode 100644 index 0000000000..b382dd59da --- /dev/null +++ b/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; diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh index bbbc8a0c97..2ce484b5db 100644 --- a/bin/tests/system/statschannel/tests.sh +++ b/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 diff --git a/configure.ac b/configure.ac index fa1dbae162..17ca8eb60c 100644 --- a/configure.ac +++ b/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 # diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index b71d323823..1d79668b91 100644 --- 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 @@ -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 diff --git a/util/copyrights b/util/copyrights index ede8082947..36a154d760 100644 --- a/util/copyrights +++ b/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