From 84ae6223b79d41ce4918928b6374baccc72c4e80 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sun, 25 Jan 2015 07:05:23 -0800 Subject: [PATCH] Revert HttpMsg::parse API from MemBuf to c-string It is simpler and more performant to incrementally convert the I/O buffers to SBuf using c-string export to legacy code than MemBuf. HttpMsg::parse API did not really make much use of MemBuf, so we can remove that and avoid performance regressions later in ICAP I/O buffer conversion. --- src/HttpMsg.cc | 20 ++++----- src/HttpMsg.h | 4 +- src/HttpReply.cc | 22 +++++----- src/HttpReply.h | 2 +- src/HttpRequest.cc | 6 +-- src/HttpRequest.h | 2 +- src/adaptation/icap/ModXact.cc | 35 +++++++++------ src/adaptation/icap/OptXact.cc | 8 ++-- src/adaptation/icap/Xaction.cc | 80 ++++++++++++++++++---------------- src/adaptation/icap/Xaction.h | 19 ++------ src/http.cc | 6 ++- src/tests/stub_HttpReply.cc | 2 +- src/tests/stub_HttpRequest.cc | 2 +- src/tests/testHttpReply.cc | 40 ++++++++--------- src/tests/testHttpRequest.cc | 12 ++--- src/tunnel.cc | 3 +- 16 files changed, 132 insertions(+), 131 deletions(-) diff --git a/src/HttpMsg.cc b/src/HttpMsg.cc index d995e3c5f3..b69c044a1c 100644 --- a/src/HttpMsg.cc +++ b/src/HttpMsg.cc @@ -125,16 +125,13 @@ httpMsgIsolateStart(const char **parse_start, const char **blk_start, const char // zero return means need more data // positive return is the size of parsed headers bool -HttpMsg::parse(MemBuf *buf, bool eof, Http::StatusCode *error) +HttpMsg::parse(const char *buf, const size_t sz, bool eof, Http::StatusCode *error) { assert(error); *error = Http::scNone; - // httpMsgParseStep() and debugging require 0-termination, unfortunately - buf->terminate(); // does not affect content size - // find the end of headers - const size_t hdr_len = headersEnd(buf->content(), buf->contentSize()); + const size_t hdr_len = headersEnd(buf, sz); // sanity check the start line to see if this is in fact an HTTP message if (!sanityCheckStartLine(buf, hdr_len, error)) { @@ -146,15 +143,14 @@ HttpMsg::parse(MemBuf *buf, bool eof, Http::StatusCode *error) return false; } - // TODO: move to httpReplyParseStep() - if (hdr_len > Config.maxReplyHeaderSize || (hdr_len <= 0 && (size_t)buf->contentSize() > Config.maxReplyHeaderSize)) { + if (hdr_len > Config.maxReplyHeaderSize || (hdr_len <= 0 && sz > Config.maxReplyHeaderSize)) { debugs(58, DBG_IMPORTANT, "HttpMsg::parse: Too large reply header (" << hdr_len << " > " << Config.maxReplyHeaderSize); *error = Http::scHeaderTooLarge; return false; } if (hdr_len <= 0) { - debugs(58, 3, "HttpMsg::parse: failed to find end of headers (eof: " << eof << ") in '" << buf->content() << "'"); + debugs(58, 3, "HttpMsg::parse: failed to find end of headers (eof: " << eof << ") in '" << buf << "'"); if (eof) // iff we have seen the end, this is an error *error = Http::scInvalidHeader; @@ -162,22 +158,22 @@ HttpMsg::parse(MemBuf *buf, bool eof, Http::StatusCode *error) return false; } - const int res = httpMsgParseStep(buf->content(), buf->contentSize(), eof); + const int res = httpMsgParseStep(buf, sz, eof); if (res < 0) { // error - debugs(58, 3, "HttpMsg::parse: cannot parse isolated headers in '" << buf->content() << "'"); + debugs(58, 3, "HttpMsg::parse: cannot parse isolated headers in '" << buf << "'"); *error = Http::scInvalidHeader; return false; } if (res == 0) { - debugs(58, 2, "HttpMsg::parse: strange, need more data near '" << buf->content() << "'"); + debugs(58, 2, "HttpMsg::parse: strange, need more data near '" << buf << "'"); *error = Http::scInvalidHeader; return false; // but this should not happen due to headersEnd() above } assert(res > 0); - debugs(58, 9, "HttpMsg::parse success (" << hdr_len << " bytes) near '" << buf->content() << "'"); + debugs(58, 9, "HttpMsg::parse success (" << hdr_len << " bytes) near '" << buf << "'"); if (hdr_sz != (int)hdr_len) { debugs(58, DBG_IMPORTANT, "internal HttpMsg::parse vs. headersEnd error: " << diff --git a/src/HttpMsg.h b/src/HttpMsg.h index 309a82972d..d85aa795b8 100644 --- a/src/HttpMsg.h +++ b/src/HttpMsg.h @@ -67,7 +67,7 @@ public: // returns true and sets hdr_sz on success // returns false and sets *error to zero when needs more data // returns false and sets *error to a positive Http::StatusCode on error - bool parse(MemBuf *buf, bool eol, Http::StatusCode *error); + bool parse(const char *buf, const size_t sz, bool eol, Http::StatusCode *error); bool parseCharBuf(const char *buf, ssize_t end); @@ -89,7 +89,7 @@ protected: * \retval true Status line has no serious problems. * \retval false Status line has a serious problem. Correct response is indicated by error. */ - virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error) = 0; + virtual bool sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error) = 0; virtual void packFirstLineInto(Packer * p, bool full_uri) const = 0; diff --git a/src/HttpReply.cc b/src/HttpReply.cc index d403616e4d..3c12fcb08d 100644 --- a/src/HttpReply.cc +++ b/src/HttpReply.cc @@ -411,15 +411,15 @@ HttpReply::bodySize(const HttpRequestMethod& method) const * NP: not all error cases are detected yet. Some are left for detection later in parse. */ bool -HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error) +HttpReply::sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error) { // hack warning: using psize instead of size here due to type mismatches with MemBuf. // content is long enough to possibly hold a reply // 4 being magic size of a 3-digit number plus space delimiter - if ( buf->contentSize() < (protoPrefix.psize() + 4) ) { + if (hdr_len < (size_t)(protoPrefix.psize() + 4)) { if (hdr_len > 0) { - debugs(58, 3, HERE << "Too small reply header (" << hdr_len << " bytes)"); + debugs(58, 3, "Too small reply header (" << hdr_len << " bytes)"); *error = Http::scInvalidHeader; } return false; @@ -428,13 +428,13 @@ HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusC int pos; // catch missing or mismatched protocol identifier // allow special-case for ICY protocol (non-HTTP identifier) in response to faked HTTP request. - if (strncmp(buf->content(), "ICY", 3) == 0) { + if (strncmp(buf, "ICY", 3) == 0) { protoPrefix = "ICY"; pos = protoPrefix.psize(); } else { - if (protoPrefix.cmp(buf->content(), protoPrefix.size()) != 0) { - debugs(58, 3, "HttpReply::sanityCheckStartLine: missing protocol prefix (" << protoPrefix << ") in '" << buf->content() << "'"); + if (protoPrefix.cmp(buf, protoPrefix.size()) != 0) { + debugs(58, 3, "missing protocol prefix (" << protoPrefix << ") in '" << buf << "'"); *error = Http::scInvalidHeader; return false; } @@ -443,21 +443,21 @@ HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusC pos = protoPrefix.psize(); // skip arbitrary number of digits and a dot in the verion portion - while ( pos <= buf->contentSize() && (*(buf->content()+pos) == '.' || xisdigit(*(buf->content()+pos)) ) ) ++pos; + while ((size_t)pos <= hdr_len && (*(buf+pos) == '.' || xisdigit(*(buf+pos)) ) ) ++pos; // catch missing version info if (pos == protoPrefix.psize()) { - debugs(58, 3, "HttpReply::sanityCheckStartLine: missing protocol version numbers (ie. " << protoPrefix << "/1.0) in '" << buf->content() << "'"); + debugs(58, 3, "missing protocol version numbers (ie. " << protoPrefix << "/1.0) in '" << buf << "'"); *error = Http::scInvalidHeader; return false; } } // skip arbitrary number of spaces... - while (pos <= buf->contentSize() && (char)*(buf->content()+pos) == ' ') ++pos; + while ((size_t)pos <= hdr_len && (char)*(buf+pos) == ' ') ++pos; - if (pos < buf->contentSize() && !xisdigit(*(buf->content()+pos))) { - debugs(58, 3, "HttpReply::sanityCheckStartLine: missing or invalid status number in '" << buf->content() << "'"); + if ((size_t)pos < hdr_len && !xisdigit(*(buf+pos))) { + debugs(58, 3, "missing or invalid status number in '" << buf << "'"); *error = Http::scInvalidHeader; return false; } diff --git a/src/HttpReply.h b/src/HttpReply.h index 05b51a346f..95aa93029b 100644 --- a/src/HttpReply.h +++ b/src/HttpReply.h @@ -39,7 +39,7 @@ public: \retval false and sets *error to zero when needs more data \retval false and sets *error to a positive Http::StatusCode on error */ - virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error); + virtual bool sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error); /** \par public, readable; never update these or their .hdr equivalents directly */ time_t date; diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 459cae8558..4b4ea77af4 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -269,11 +269,11 @@ HttpRequest::inheritProperties(const HttpMsg *aMsg) * NP: Other errors are left for detection later in the parse. */ bool -HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error) +HttpRequest::sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error) { // content is long enough to possibly hold a reply // 2 being magic size of a 1-byte request method plus space delimiter - if ( buf->contentSize() < 2 ) { + if (hdr_len < 2) { // this is ony a real error if the headers apparently complete. if (hdr_len > 0) { debugs(58, 3, HERE << "Too large request header (" << hdr_len << " bytes)"); @@ -284,7 +284,7 @@ HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::Statu /* See if the request buffer starts with a non-whitespace HTTP request 'method'. */ HttpRequestMethod m; - m.HttpRequestMethodXXX(buf->content()); + m.HttpRequestMethodXXX(buf); if (m == Http::METHOD_NONE) { debugs(73, 3, "HttpRequest::sanityCheckStartLine: did not find HTTP request method"); *error = Http::scInvalidHeader; diff --git a/src/HttpRequest.h b/src/HttpRequest.h index 73f3065363..348e19825a 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -257,7 +257,7 @@ private: protected: virtual void packFirstLineInto(Packer * p, bool full_uri) const; - virtual bool sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error); + virtual bool sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error); virtual void hdrCacheInit(); diff --git a/src/adaptation/icap/ModXact.cc b/src/adaptation/icap/ModXact.cc index d8a3b4106c..a8375a7ee6 100644 --- a/src/adaptation/icap/ModXact.cc +++ b/src/adaptation/icap/ModXact.cc @@ -557,10 +557,10 @@ void Adaptation::Icap::ModXact::readMore() return; } - if (readBuf.hasSpace()) + if (readBuf.spaceSize()) scheduleRead(); else - debugs(93,3,HERE << "nothing to do because !readBuf.hasSpace()"); + debugs(93,3,HERE << "nothing to do because !readBuf.spaceSize()"); } // comm module read a portion of the ICAP response for us @@ -649,9 +649,8 @@ void Adaptation::Icap::ModXact::checkConsuming() void Adaptation::Icap::ModXact::parseMore() { - debugs(93, 5, HERE << "have " << readBuf.contentSize() << " bytes to parse" << - status()); - debugs(93, 5, HERE << "\n" << readBuf.content()); + debugs(93, 5, "have " << readBuf.length() << " bytes to parse" << status()); + debugs(93, 5, "\n" << readBuf); if (state.parsingHeaders()) parseHeaders(); @@ -967,7 +966,8 @@ void Adaptation::Icap::ModXact::prepEchoing() // parse the buffer back Http::StatusCode error = Http::scNone; - Must(adapted.header->parse(&httpBuf, true, &error)); + httpBuf.terminate(); // HttpMsg::parse requires nil-terminated buffer + Must(adapted.header->parse(httpBuf.content(), httpBuf.contentSize(), true, &error)); if (HttpRequest *r = dynamic_cast(adapted.header)) urlCanonical(r); // parse does not set HttpRequest::canonical @@ -1075,11 +1075,13 @@ void Adaptation::Icap::ModXact::parseHttpHead() bool Adaptation::Icap::ModXact::parseHead(HttpMsg *head) { Must(head); - debugs(93, 5, HERE << "have " << readBuf.contentSize() << " head bytes to parse" << - "; state: " << state.parsing); + debugs(93, 5, "have " << readBuf.length() << " head bytes to parse; state: " << state.parsing); Http::StatusCode error = Http::scNone; - const bool parsed = head->parse(&readBuf, commEof, &error); + // XXX: performance regression. c_str() data copies + // XXX: HttpMsg::parse requires a terminated string buffer + const char *tmpBuf = readBuf.c_str(); + const bool parsed = head->parse(tmpBuf, readBuf.length(), commEof, &error); Must(parsed || !error); // success or need more data if (!parsed) { // need more data @@ -1117,15 +1119,22 @@ void Adaptation::Icap::ModXact::parseBody() Must(state.parsing == State::psBody); Must(bodyParser); - debugs(93, 5, HERE << "have " << readBuf.contentSize() << " body bytes to parse"); + debugs(93, 5, "have " << readBuf.length() << " body bytes to parse"); // the parser will throw on errors BodyPipeCheckout bpc(*adapted.body_pipe); - const bool parsed = bodyParser->parse(&readBuf, &bpc.buf); + // XXX: performance regression. SBuf-convert (or Parser-convert?) the chunked decoder. + MemBuf encodedData; + encodedData.init(); + // NP: we must do this instead of pointing encodedData at the SBuf::rawContent + // because chunked decoder uses MemBuf::consume, which shuffles buffer bytes around. + encodedData.append(readBuf.rawContent(), readBuf.length()); + const bool parsed = bodyParser->parse(&encodedData, &bpc.buf); + // XXX: httpChunkDecoder has consumed from MemBuf. + readBuf.consume(readBuf.length() - encodedData.contentSize()); bpc.checkIn(); - debugs(93, 5, HERE << "have " << readBuf.contentSize() << " body bytes after " << - "parse; parsed all: " << parsed); + debugs(93, 5, "have " << readBuf.length() << " body bytes after parsed all: " << parsed); replyHttpBodySize += adapted.body_pipe->buf().contentSize(); // TODO: expose BodyPipe::putSize() to make this check simpler and clearer diff --git a/src/adaptation/icap/OptXact.cc b/src/adaptation/icap/OptXact.cc index 103b2968fb..5aa1225441 100644 --- a/src/adaptation/icap/OptXact.cc +++ b/src/adaptation/icap/OptXact.cc @@ -67,7 +67,8 @@ void Adaptation::Icap::OptXact::makeRequest(MemBuf &buf) // XXX: HttpRequest cannot fully parse ICAP Request-Line Http::StatusCode reqStatus; - Must(icapRequest->parse(&buf, true, &reqStatus) > 0); + buf.terminate(); // HttpMsg::parse requires terminated buffer + Must(icapRequest->parse(buf.content(), buf.contentSize(), true, &reqStatus) > 0); } void Adaptation::Icap::OptXact::handleCommWrote(size_t size) @@ -99,9 +100,8 @@ void Adaptation::Icap::OptXact::handleCommRead(size_t) bool Adaptation::Icap::OptXact::parseResponse() { - debugs(93, 5, HERE << "have " << readBuf.contentSize() << " bytes to parse" << - status()); - debugs(93, 5, HERE << "\n" << readBuf.content()); + debugs(93, 5, "have " << readBuf.length() << " bytes to parse" << status()); + debugs(93, DBG_DATA, "\n" << readBuf); HttpReply::Pointer r(new HttpReply); r->protoPrefix = "ICAP/"; // TODO: make an IcapReply class? diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index 8dee87fad1..6485ac2f3b 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -39,8 +39,6 @@ Adaptation::Icap::Xaction::Xaction(const char *aTypeName, Adaptation::Icap::Serv attempts(0), connection(NULL), theService(aService), - commBuf(NULL), - commBufSize(0), commEof(false), reuseConnection(true), isRetriable(true), @@ -95,11 +93,6 @@ void Adaptation::Icap::Xaction::disableRepeats(const char *reason) void Adaptation::Icap::Xaction::start() { Adaptation::Initiate::start(); - - readBuf.init(SQUID_TCP_SO_RCVBUF, SQUID_TCP_SO_RCVBUF); - commBuf = (char*)memAllocBuf(SQUID_TCP_SO_RCVBUF, &commBufSize); - // make sure maximum readBuf space does not exceed commBuf size - Must(static_cast(readBuf.potentialSpaceSize()) <= commBufSize); } static void @@ -383,17 +376,14 @@ void Adaptation::Icap::Xaction::scheduleRead() { Must(haveConnection()); Must(!reader); - Must(readBuf.hasSpace()); - /* - * See comments in Adaptation::Icap::Xaction.h about why we use commBuf - * here instead of reading directly into readBuf.buf. - */ - typedef CommCbMemFunT Dialer; - reader = JobCallback(93, 3, - Dialer, this, Adaptation::Icap::Xaction::noteCommRead); + // TODO: tune this better to expected message sizes + readBuf.reserveCapacity(SQUID_TCP_SO_RCVBUF); + Must(readBuf.spaceSize()); - comm_read(connection, commBuf, readBuf.spaceSize(), reader); + typedef CommCbMemFunT Dialer; + reader = JobCallback(93, 3, Dialer, this, Adaptation::Icap::Xaction::noteCommRead); + Comm::Read(connection, reader); updateTimeout(); } @@ -405,7 +395,30 @@ void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io) Must(io.flag == Comm::OK); - if (!io.size) { + CommIoCbParams rd(this); // will be expanded with ReadNow results + rd.conn = io.conn; + rd.size = readBuf.spaceSize(); + + switch (Comm::ReadNow(rd, readBuf)) { // XXX: SBuf convert readBuf + case Comm::INPROGRESS: + if (readBuf.isEmpty()) + debugs(33, 2, io.conn << ": no data to process, " << xstrerr(rd.xerrno)); + scheduleRead(); + return; + + case Comm::OK: + al.icap.bytesRead += rd.size; + + updateTimeout(); + + debugs(93, 3, "read " << rd.size << " bytes"); + + disableRetries(); // because pconn did not fail + + /* Continue to process previously read data */ + break; + + case Comm::ENDFILE: // close detected by 0-byte read commEof = true; reuseConnection = false; @@ -415,21 +428,14 @@ void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io) mustStop("pconn race"); return; } - } else { - - al.icap.bytesRead+=io.size; - - updateTimeout(); - - debugs(93, 3, HERE << "read " << io.size << " bytes"); - /* - * See comments in Adaptation::Icap::Xaction.h about why we use commBuf - * here instead of reading directly into readBuf.buf. - */ + break; - readBuf.append(commBuf, io.size); - disableRetries(); // because pconn did not fail + // case Comm::COMM_ERROR: + default: // no other flags should ever occur + debugs(11, 2, io.conn << ": read failure: " << xstrerr(rd.xerrno)); + mustStop("unknown ICAP I/O read error"); + return; } handleCommRead(io.size); @@ -446,10 +452,12 @@ void Adaptation::Icap::Xaction::cancelRead() bool Adaptation::Icap::Xaction::parseHttpMsg(HttpMsg *msg) { - debugs(93, 5, HERE << "have " << readBuf.contentSize() << " head bytes to parse"); + debugs(93, 5, "have " << readBuf.length() << " head bytes to parse"); Http::StatusCode error = Http::scNone; - const bool parsed = msg->parse(&readBuf, commEof, &error); + // XXX: performance regression c_str() data copies + const char *buf = readBuf.c_str(); + const bool parsed = msg->parse(buf, readBuf.length(), commEof, &error); Must(parsed || !error); // success or need more data if (!parsed) { // need more data @@ -465,7 +473,7 @@ bool Adaptation::Icap::Xaction::parseHttpMsg(HttpMsg *msg) bool Adaptation::Icap::Xaction::mayReadMore() const { return !doneReading() && // will read more data - readBuf.hasSpace(); // have space for more data + readBuf.spaceSize(); // have space for more data } bool Adaptation::Icap::Xaction::doneReading() const @@ -530,11 +538,7 @@ void Adaptation::Icap::Xaction::swanSong() closeConnection(); // TODO: rename because we do not always close - if (!readBuf.isNull()) - readBuf.clean(); - - if (commBuf) - memFreeBuf(commBufSize, commBuf); + readBuf.clear(); tellQueryAborted(); diff --git a/src/adaptation/icap/Xaction.h b/src/adaptation/icap/Xaction.h index 644449521c..d5b812f6b6 100644 --- a/src/adaptation/icap/Xaction.h +++ b/src/adaptation/icap/Xaction.h @@ -16,7 +16,9 @@ #include "CommCalls.h" #include "HttpReply.h" #include "ipcache.h" -#include "MemBuf.h" +#include "SBuf.h" + +class MemBuf; // XXX: may not be needed when we are done SBuf'ing namespace Adaptation { @@ -127,20 +129,7 @@ protected: Comm::ConnectionPointer connection; ///< ICAP server connection Adaptation::Icap::ServiceRep::Pointer theService; - /* - * We have two read buffers. We would prefer to read directly - * into the MemBuf, but since comm_read isn't MemBuf-aware, and - * uses event-delayed callbacks, it leaves the MemBuf in an - * inconsistent state. There would be data in the buffer, but - * MemBuf.size won't be updated until the (delayed) callback - * occurs. To avoid that situation we use a plain buffer - * (commBuf) and then copy (append) its contents to readBuf in - * the callback. If comm_read ever becomes MemBuf-aware, we - * can eliminate commBuf and this extra buffer copy. - */ - MemBuf readBuf; - char *commBuf; - size_t commBufSize; + SBuf readBuf; bool commEof; bool reuseConnection; bool isRetriable; ///< can retry on persistent connection failures diff --git a/src/http.cc b/src/http.cc index 1757073d7f..665b505714 100644 --- a/src/http.cc +++ b/src/http.cc @@ -705,7 +705,8 @@ HttpStateData::processReplyHeader() Http::StatusCode error = Http::scNone; HttpReply *newrep = new HttpReply; - const bool parsed = newrep->parse(readBuf, eof, &error); + readBuf->terminate(); // XXX: HttpMsg::parse requires terminated string + const bool parsed = newrep->parse(readBuf->content(), readBuf->contentSize(), eof, &error); if (!parsed && readBuf->contentSize() > 5 && strncmp(readBuf->content(), "HTTP/", 5) != 0 && strncmp(readBuf->content(), "ICY", 3) != 0) { MemBuf *mb; @@ -713,7 +714,8 @@ HttpStateData::processReplyHeader() tmprep->setHeaders(Http::scOkay, "Gatewaying", NULL, -1, -1, -1); tmprep->header.putExt("X-Transformed-From", "HTTP/0.9"); mb = tmprep->pack(); - newrep->parse(mb, eof, &error); + mb->terminate(); // XXX: HttpMsg::parse requires terminated string + newrep->parse(mb->content(), mb->contentSize(), eof, &error); delete mb; delete tmprep; } else { diff --git a/src/tests/stub_HttpReply.cc b/src/tests/stub_HttpReply.cc index d1d1a77dc5..8ae8d38863 100644 --- a/src/tests/stub_HttpReply.cc +++ b/src/tests/stub_HttpReply.cc @@ -21,7 +21,7 @@ HttpReply::HttpReply() : HttpMsg(hoReply), date (0), last_modified (0), void HttpReply::packHeadersInto(Packer * p) const STUB void HttpReply::reset() STUB void httpBodyPackInto(const HttpBody * body, Packer * p) STUB - bool HttpReply::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false) + bool HttpReply::sanityCheckStartLine(const char *buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false) int HttpReply::httpMsgParseError() STUB_RETVAL(0) bool HttpReply::expectingBody(const HttpRequestMethod&, int64_t&) const STUB_RETVAL(false) bool HttpReply::parseFirstLine(const char *start, const char *end) STUB_RETVAL(false) diff --git a/src/tests/stub_HttpRequest.cc b/src/tests/stub_HttpRequest.cc index fcaf21d62b..62ff173da5 100644 --- a/src/tests/stub_HttpRequest.cc +++ b/src/tests/stub_HttpRequest.cc @@ -17,7 +17,7 @@ HttpRequest::HttpRequest() : HttpMsg(hoRequest) STUB HttpRequest::HttpRequest(const HttpRequestMethod& aMethod, AnyP::ProtocolType aProtocol, const char *aUrlpath) : HttpMsg(hoRequest) STUB HttpRequest::~HttpRequest() STUB void HttpRequest::packFirstLineInto(Packer * p, bool full_uri) const STUB - bool HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false) + bool HttpRequest::sanityCheckStartLine(const char*buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false) void HttpRequest::hdrCacheInit() STUB void HttpRequest::reset() STUB bool HttpRequest::expectingBody(const HttpRequestMethod& unused, int64_t&) const STUB_RETVAL(false) diff --git a/src/tests/testHttpReply.cc b/src/tests/testHttpReply.cc index a025909036..8fcd9d6d29 100644 --- a/src/tests/testHttpReply.cc +++ b/src/tests/testHttpReply.cc @@ -50,14 +50,14 @@ testHttpReply::testSanityCheckFirstLine() // a valid status line input.append("HTTP/1.1 200 Okay\n\n", 19); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT( 1 && engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT( 1 && engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("HTTP/1.1 200 Okay \n\n", 28); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT( 2 && engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT( 2 && engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; @@ -66,14 +66,14 @@ testHttpReply::testSanityCheckFirstLine() // invalid status line input.append("HTTP/1.1 999 Okay\n\n", 19); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT( 3 && !engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT( 3 && !engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; input.append("HTTP/1.1 2000 Okay \n\n", 29); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT( 4 && engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT( 4 && engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; @@ -82,7 +82,7 @@ testHttpReply::testSanityCheckFirstLine() // valid ICY protocol status line input.append("ICY 200 Okay\n\n", 14); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT( engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT( engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; @@ -93,14 +93,14 @@ testHttpReply::testSanityCheckFirstLine() // empty status line input.append("\n\n", 2); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT( 5 && !engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT( 5 && !engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; input.append(" \n\n", 8); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT( 6 && !engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT( 6 && !engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; @@ -108,14 +108,14 @@ testHttpReply::testSanityCheckFirstLine() // status line with no message input.append("HTTP/1.1 200\n\n", 14); /* real case seen */ hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("HTTP/1.1 200 \n\n", 15); /* real case seen */ hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; @@ -123,42 +123,42 @@ testHttpReply::testSanityCheckFirstLine() // incomplete (short) status lines... not sane (yet), but no error either. input.append("H", 1); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("HTTP/", 5); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("HTTP/1", 6); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("HTTP/1.1", 8); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("HTTP/1.1 ", 9); /* real case seen */ hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("HTTP/1.1 20", 14); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; @@ -166,21 +166,21 @@ testHttpReply::testSanityCheckFirstLine() // status line with no status input.append("HTTP/1.1 \n\n", 11); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; input.append("HTTP/1.1 \n\n", 15); hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; input.append("HTTP/1.1 Okay\n\n", 16); /* real case seen */ hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; @@ -188,7 +188,7 @@ testHttpReply::testSanityCheckFirstLine() // status line with nul-byte input.append("HTTP/1.1" "\0" "200 Okay\n\n", 19); /* real case seen */ hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; @@ -196,7 +196,7 @@ testHttpReply::testSanityCheckFirstLine() // status line with negative status input.append("HTTP/1.1 -000\n\n", 15); /* real case seen */ hdr_len = headersEnd(input.content(),input.contentSize()); - CPPUNIT_ASSERT(!engine.sanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.sanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; diff --git a/src/tests/testHttpRequest.cc b/src/tests/testHttpRequest.cc index 336ae3265c..aacda2df55 100644 --- a/src/tests/testHttpRequest.cc +++ b/src/tests/testHttpRequest.cc @@ -22,7 +22,7 @@ CPPUNIT_TEST_SUITE_REGISTRATION( testHttpRequest ); class PrivateHttpRequest : public HttpRequest { public: - bool doSanityCheckStartLine(MemBuf *b, const size_t h, Http::StatusCode *e) { return sanityCheckStartLine(b,h,e); }; + bool doSanityCheckStartLine(const char *b, const size_t h, Http::StatusCode *e) { return sanityCheckStartLine(b,h,e); }; }; /* init memory pools */ @@ -164,14 +164,14 @@ testHttpRequest::testSanityCheckStartLine() // a valid request line input.append("GET / HTTP/1.1\n\n", 16); hdr_len = headersEnd(input.content(), input.contentSize()); - CPPUNIT_ASSERT(engine.doSanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(engine.doSanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("GET / HTTP/1.1\n\n", 18); hdr_len = headersEnd(input.content(), input.contentSize()); - CPPUNIT_ASSERT(engine.doSanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(engine.doSanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; @@ -179,14 +179,14 @@ testHttpRequest::testSanityCheckStartLine() // strange but valid methods input.append(". / HTTP/1.1\n\n", 14); hdr_len = headersEnd(input.content(), input.contentSize()); - CPPUNIT_ASSERT(engine.doSanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(engine.doSanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; input.append("OPTIONS * HTTP/1.1\n\n", 20); hdr_len = headersEnd(input.content(), input.contentSize()); - CPPUNIT_ASSERT(engine.doSanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(engine.doSanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scNone); input.reset(); error = Http::scNone; @@ -203,7 +203,7 @@ testHttpRequest::testSanityCheckStartLine() input.append(" \n\n", 8); hdr_len = headersEnd(input.content(), input.contentSize()); - CPPUNIT_ASSERT(!engine.doSanityCheckStartLine(&input, hdr_len, &error) ); + CPPUNIT_ASSERT(!engine.doSanityCheckStartLine(input.content(), hdr_len, &error) ); CPPUNIT_ASSERT_EQUAL(error, Http::scInvalidHeader); input.reset(); error = Http::scNone; diff --git a/src/tunnel.cc b/src/tunnel.cc index fec72681ad..4dda5d2fea 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -399,7 +399,8 @@ TunnelStateData::handleConnectResponse(const size_t chunkSize) HttpReply rep; Http::StatusCode parseErr = Http::scNone; const bool eof = !chunkSize; - const bool parsed = rep.parse(connectRespBuf, eof, &parseErr); + connectRespBuf->terminate(); // HttpMsg::parse requires terminated string + const bool parsed = rep.parse(connectRespBuf->content(), connectRespBuf->contentSize(), eof, &parseErr); if (!parsed) { if (parseErr > 0) { // unrecoverable parsing error server.logicError("malformed CONNECT response from peer"); -- 2.39.2