[squid-dev] [PATCH] ICAP read buffer SBuf conversion
Amos Jeffries
squid3 at treenet.co.nz
Sat Feb 28 04:15:10 UTC 2015
Contains three changes required before we can upgrade the ICAP message
parser:
* Convert the ICAP read buffer to an SBuf.
* Remove the double-buffering hack used to comm_read() ICAP responses as
c-string then convert to MemBuf for parsing.
* Revert the HttpMsg parser API from MemBuf to c-string parameters.
The internals did not make much use of the MemBuf abilities and it is
simpler to retrieve c-string values directly from an SBuf than to go via
a MemBuf conversion.
I am seeking help testing that the ICAP I/O is still operational. If
anyone has ICAP services and is willing to run a trunk/3.HEAD proxy with
this patch applied please do.
Amos
-------------- next part --------------
=== modified file 'src/HttpMsg.cc'
--- src/HttpMsg.cc 2015-01-13 07:25:36 +0000
+++ src/HttpMsg.cc 2015-01-25 10:33:14 +0000
@@ -108,93 +108,89 @@
*blk_start = *parse_start;
*blk_end = *blk_start + slen;
while (**blk_end == '\r') /* CR */
++(*blk_end);
if (**blk_end == '\n') /* LF */
++(*blk_end);
*parse_start = *blk_end;
return 1;
}
// negative return is the negated Http::StatusCode error code
// 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)) {
// NP: sanityCheck sets *error and sends debug warnings on syntax errors.
// if we have seen the connection close, this is an error too
if (eof && *error == Http::scNone)
*error = Http::scInvalidHeader;
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;
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: " <<
hdr_sz << " != " << hdr_len);
hdr_sz = (int)hdr_len; // because old http.cc code used hdr_len
}
return true;
}
/*
* parseCharBuf() takes character buffer of HTTP headers (buf),
* which may not be NULL-terminated, and fills in an HttpMsg
* structure. The parameter 'end' specifies the offset to
* the end of the reply headers. The caller may know where the
* end is, but is unable to NULL-terminate the buffer. This function
* returns true on success.
*/
bool
HttpMsg::parseCharBuf(const char *buf, ssize_t end)
=== modified file 'src/HttpMsg.h'
--- src/HttpMsg.h 2015-01-13 07:25:36 +0000
+++ src/HttpMsg.h 2015-01-25 09:56:54 +0000
@@ -50,56 +50,56 @@
AnyP::ProtocolVersion http_ver;
HttpHeader header;
HttpHdrCc *cache_control;
/* Unsupported, writable, may disappear/change in the future
* For replies, sums _stored_ status-line, headers, and <CRLF>.
* Also used to report parsed header size if parse() is successful */
int hdr_sz;
int64_t content_length;
HttpMsgParseState pstate; /* the current parsing state */
BodyPipe::Pointer body_pipe; // optional pipeline to receive message body
// 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);
int httpMsgParseStep(const char *buf, int len, int atEnd);
virtual int httpMsgParseError();
virtual bool expectingBody(const HttpRequestMethod&, int64_t&) const = 0;
void firstLineBuf(MemBuf&);
virtual bool inheritProperties(const HttpMsg *aMsg) = 0;
protected:
/**
* Validate the message start line is syntactically correct.
* Set HTTP error status according to problems found.
*
* \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;
virtual bool parseFirstLine(const char *blk_start, const char *blk_end) = 0;
virtual void hdrCacheInit();
};
#define HTTPMSGUNLOCK(a) if (a) { if ((a)->unlock() == 0) delete (a); (a)=NULL; }
#define HTTPMSGLOCK(a) (a)->lock()
#endif /* SQUID_HTTPMSG_H */
=== modified file 'src/HttpReply.cc'
--- src/HttpReply.cc 2015-01-13 07:25:36 +0000
+++ src/HttpReply.cc 2015-01-25 10:40:11 +0000
@@ -394,87 +394,87 @@
return 0;
else if (sline.status() == Http::scOkay)
(void) 0; /* common case, continue */
else if (sline.status() == Http::scNoContent)
return 0;
else if (sline.status() == Http::scNotModified)
return 0;
else if (sline.status() < Http::scOkay)
return 0;
return content_length;
}
/**
* Checks the first line of an HTTP Reply is valid.
* currently only checks "HTTP/" exists.
*
* 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;
}
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;
}
// catch missing or negative status value (negative '-' is not a digit)
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;
}
return true;
}
bool
HttpReply::parseFirstLine(const char *blk_start, const char *blk_end)
{
return sline.parse(protoPrefix, blk_start, blk_end);
}
/* handy: resets and returns -1 */
int
HttpReply::httpMsgParseError()
{
int result(HttpMsg::httpMsgParseError());
/* indicate an error in the status line */
sline.set(Http::ProtocolVersion(), Http::scInvalidHeader);
=== modified file 'src/HttpReply.h'
--- src/HttpReply.h 2015-01-13 07:25:36 +0000
+++ src/HttpReply.h 2015-01-25 15:04:40 +0000
@@ -22,41 +22,41 @@
class HttpHdrSc;
class HttpReply: public HttpMsg
{
MEMPROXY_CLASS(HttpReply);
public:
typedef RefCount<HttpReply> Pointer;
HttpReply();
~HttpReply();
virtual void reset();
/**
\retval true on success
\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;
time_t last_modified;
time_t expires;
String content_type;
HttpHdrSc *surrogate_control;
HttpHdrContRange *content_range;
short int keep_alive;
/** \par public, writable, but use httpReply* interfaces when possible */
Http::StatusLine sline;
HttpBody body; /**< for small constant memory-resident text bodies only */
=== modified file 'src/HttpRequest.cc'
--- src/HttpRequest.cc 2015-01-29 16:09:11 +0000
+++ src/HttpRequest.cc 2015-02-09 14:04:37 +0000
@@ -252,56 +252,56 @@
#endif
myportname = aReq->myportname;
forcedBodyContinuation = aReq->forcedBodyContinuation;
// main property is which connection the request was received on (if any)
clientConnectionManager = aReq->clientConnectionManager;
notes = aReq->notes;
return true;
}
/**
* Checks the first line of an HTTP request is valid
* currently just checks the request method is present.
*
* 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)");
*error = Http::scInvalidHeader;
}
return false;
}
/* 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;
return false;
}
return true;
}
bool
HttpRequest::parseFirstLine(const char *start, const char *end)
{
method.HttpRequestMethodXXX(start);
if (method == Http::METHOD_NONE)
return false;
// XXX: performance regression, strcspn() over the method bytes a second time.
// cheaper than allocate+copy+deallocate cycle to SBuf convert a piece of start.
const char *t = start + strcspn(start, w_space);
=== modified file 'src/HttpRequest.h'
--- src/HttpRequest.h 2015-01-29 16:09:11 +0000
+++ src/HttpRequest.h 2015-02-09 14:04:37 +0000
@@ -240,29 +240,29 @@
/**
* The client connection manager, if known;
* Used for any response actions needed directly to the client.
* ie 1xx forwarding or connection pinning state changes
*/
CbcPointer<ConnStateData> clientConnectionManager;
/// forgets about the cached Range header (for a reason)
void ignoreRange(const char *reason);
int64_t getRangeOffsetLimit(); /* the result of this function gets cached in rangeOffsetLimit */
private:
const char *packableURI(bool full_uri) const;
mutable int64_t rangeOffsetLimit; /* caches the result of getRangeOffsetLimit */
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();
virtual bool inheritProperties(const HttpMsg *aMsg);
};
#endif /* SQUID_HTTPREQUEST_H */
=== modified file 'src/adaptation/icap/ModXact.cc'
--- src/adaptation/icap/ModXact.cc 2015-01-13 07:25:36 +0000
+++ src/adaptation/icap/ModXact.cc 2015-01-24 14:50:13 +0000
@@ -540,44 +540,44 @@
Must(!adapted.body_pipe);
// we use the same buffer for headers and body and then consume headers
readMore();
}
void Adaptation::Icap::ModXact::readMore()
{
if (reader != NULL || doneReading()) {
debugs(93,3,HERE << "returning from readMore because reader or doneReading()");
return;
}
// do not fill readBuf if we have no space to store the result
if (adapted.body_pipe != NULL &&
!adapted.body_pipe->buf().hasPotentialSpace()) {
debugs(93,3,HERE << "not reading because ICAP reply pipe is full");
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
void Adaptation::Icap::ModXact::handleCommRead(size_t)
{
Must(!state.doneParsing());
icap_tio_finish = current_time;
parseMore();
readMore();
}
void Adaptation::Icap::ModXact::echoMore()
{
Must(state.sending == State::sendingVirgin);
Must(adapted.body_pipe != NULL);
Must(virginBodySending.active());
const size_t sizeMax = virginContentSize(virginBodySending);
debugs(93,5, HERE << "will echo up to " << sizeMax << " bytes from " <<
virgin.body_pipe->status());
@@ -632,43 +632,42 @@
Must(!adapted.body_pipe);
}
state.sending = State::sendingDone;
checkConsuming();
}
// should be called after certain state.writing or state.sending changes
void Adaptation::Icap::ModXact::checkConsuming()
{
// quit if we already stopped or are still using the pipe
if (!virgin.body_pipe || !state.doneConsumingVirgin())
return;
debugs(93, 7, HERE << "will stop consuming" << status());
stopConsumingFrom(virgin.body_pipe);
}
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();
if (state.parsing == State::psBody)
parseBody();
}
void Adaptation::Icap::ModXact::callException(const std::exception &e)
{
if (!canStartBypass || isRetriable) {
if (!isRetriable) {
if (const TextException *te = dynamic_cast<const TextException *>(&e))
detailError(ERR_DETAIL_EXCEPTION_START + te->id());
else
detailError(ERR_DETAIL_EXCEPTION_OTHER);
}
Adaptation::Icap::Xaction::callException(e);
return;
}
@@ -950,41 +949,42 @@
// allocate the adapted message and copy metainfo
Must(!adapted.header);
{
HttpMsg::Pointer newHead;
if (dynamic_cast<const HttpRequest*>(oldHead)) {
newHead = new HttpRequest;
} else if (dynamic_cast<const HttpReply*>(oldHead)) {
newHead = new HttpReply;
}
Must(newHead != NULL);
newHead->inheritProperties(oldHead);
adapted.setHeader(newHead.getRaw());
}
// 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<HttpRequest*>(adapted.header))
urlCanonical(r); // parse does not set HttpRequest::canonical
Must(adapted.header->hdr_sz == httpBuf.contentSize()); // no leftovers
httpBuf.clean();
debugs(93, 7, HERE << "cloned virgin message " << oldHead << " to " <<
adapted.header);
// setup adapted body pipe if needed
if (oldHead->body_pipe != NULL) {
debugs(93, 7, HERE << "will echo virgin body from " <<
oldHead->body_pipe);
if (!virginBodySending.active())
virginBodySending.plan(); // will throw if not possible
state.sending = State::sendingVirgin;
checkConsuming();
@@ -1058,91 +1058,100 @@
const HttpRequest *oldR = dynamic_cast<const HttpRequest*>(virgin.header);
Must(oldR);
// TODO: the adapted request did not really originate from the
// client; give proxy admin an option to prevent copying of
// sensitive client information here. See the following thread:
// http://www.squid-cache.org/mail-archive/squid-dev/200703/0040.html
}
// Maybe adapted.header==NULL if HttpReply and have Http 0.9 ....
if (adapted.header)
adapted.header->inheritProperties(virgin.header);
}
decideOnParsingBody();
}
// parses both HTTP and ICAP headers
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
debugs(93, 5, HERE << "parse failed, need more data, return false");
head->reset();
return false;
}
if (HttpRequest *r = dynamic_cast<HttpRequest*>(head))
urlCanonical(r); // parse does not set HttpRequest::canonical
debugs(93, 5, HERE << "parse success, consume " << head->hdr_sz << " bytes, return true");
readBuf.consume(head->hdr_sz);
return true;
}
void Adaptation::Icap::ModXact::decideOnParsingBody()
{
if (gotEncapsulated("res-body") || gotEncapsulated("req-body")) {
debugs(93, 5, HERE << "expecting a body");
state.parsing = State::psBody;
replyHttpBodySize = 0;
bodyParser = new ChunkedCodingParser;
makeAdaptedBodyPipe("adapted response from the ICAP server");
Must(state.sending == State::sendingAdapted);
} else {
debugs(93, 5, HERE << "not expecting a body");
stopParsing();
stopSending(true);
}
}
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
// TODO: do we really need this if we disable when sending headers?
if (adapted.body_pipe->buf().contentSize() > 0) { // parsed something sometime
disableRepeats("sent adapted content");
disableBypass("sent adapted content", true);
}
if (parsed) {
if (state.readyForUob && bodyParser->useOriginBody >= 0) {
prepPartialBodyEchoing(
static_cast<uint64_t>(bodyParser->useOriginBody));
stopParsing();
return;
}
stopParsing();
stopSending(true); // the parser succeeds only if all parsed data fits
return;
=== modified file 'src/adaptation/icap/OptXact.cc'
--- src/adaptation/icap/OptXact.cc 2015-01-13 07:25:36 +0000
+++ src/adaptation/icap/OptXact.cc 2015-01-23 15:15:24 +0000
@@ -50,75 +50,75 @@
scheduleWrite(requestBuf);
}
void Adaptation::Icap::OptXact::makeRequest(MemBuf &buf)
{
const Adaptation::Service &s = service();
const String uri = s.cfg().uri;
buf.Printf("OPTIONS " SQUIDSTRINGPH " ICAP/1.0\r\n", SQUIDSTRINGPRINT(uri));
const String host = s.cfg().host;
buf.Printf("Host: " SQUIDSTRINGPH ":%d\r\n", SQUIDSTRINGPRINT(host), s.cfg().port);
if (!TheConfig.reuse_connections)
buf.Printf("Connection: close\r\n");
if (TheConfig.allow206_enable)
buf.Printf("Allow: 206\r\n");
buf.append(ICAP::crlf, 2);
// 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)
{
debugs(93, 9, HERE << "finished writing " << size <<
"-byte request " << status());
}
// comm module read a portion of the ICAP response for us
void Adaptation::Icap::OptXact::handleCommRead(size_t)
{
if (parseResponse()) {
Must(icapReply != NULL);
// We read everything if there is no response body. If there is a body,
// we cannot parse it because we do not support any opt-body-types, so
// we leave readAll false which forces connection closure.
readAll = !icapReply->header.getByNameListMember("Encapsulated",
"opt-body", ',').size();
debugs(93, 7, HERE << "readAll=" << readAll);
icap_tio_finish = current_time;
setOutcome(xoOpt);
sendAnswer(Answer::Forward(icapReply.getRaw()));
Must(done()); // there should be nothing else to do
return;
}
scheduleRead();
}
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?
if (!parseHttpMsg(r.getRaw())) // throws on errors
return false;
if (httpHeaderHasConnDir(&r->header, "close"))
reuseConnection = false;
icapReply = r;
return true;
}
void Adaptation::Icap::OptXact::swanSong()
{
Adaptation::Icap::Xaction::swanSong();
}
void Adaptation::Icap::OptXact::finalizeLogInfo()
=== modified file 'src/adaptation/icap/Xaction.cc'
--- src/adaptation/icap/Xaction.cc 2015-01-29 16:09:11 +0000
+++ src/adaptation/icap/Xaction.cc 2015-02-09 14:04:37 +0000
@@ -22,42 +22,40 @@
#include "err_detail_type.h"
#include "fde.h"
#include "FwdState.h"
#include "HttpMsg.h"
#include "HttpReply.h"
#include "HttpRequest.h"
#include "icap_log.h"
#include "ipcache.h"
#include "pconn.h"
#include "SquidConfig.h"
#include "SquidTime.h"
Adaptation::Icap::Xaction::Xaction(const char *aTypeName, Adaptation::Icap::ServiceRep::Pointer &aService):
AsyncJob(aTypeName),
Adaptation::Initiate(aTypeName),
icapRequest(NULL),
icapReply(NULL),
attempts(0),
connection(NULL),
theService(aService),
- commBuf(NULL),
- commBufSize(0),
commEof(false),
reuseConnection(true),
isRetriable(true),
isRepeatable(true),
ignoreLastWrite(false),
stopReason(NULL),
connector(NULL),
reader(NULL),
writer(NULL),
closer(NULL),
alep(new AccessLogEntry),
al(*alep),
cs(NULL)
{
debugs(93,3, typeName << " constructed, this=" << this <<
" [icapx" << id << ']'); // we should not call virtual status() here
icapRequest = new HttpRequest;
HTTPMSGLOCK(icapRequest);
icap_tr_start = current_time;
memset(&icap_tio_start, 0, sizeof(icap_tio_start));
@@ -78,45 +76,40 @@
return *theService;
}
void Adaptation::Icap::Xaction::disableRetries()
{
debugs(93,5, typeName << (isRetriable ? " from now on" : " still") <<
" cannot be retried " << status());
isRetriable = false;
}
void Adaptation::Icap::Xaction::disableRepeats(const char *reason)
{
debugs(93,5, typeName << (isRepeatable ? " from now on" : " still") <<
" cannot be repeated because " << reason << status());
isRepeatable = false;
}
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<size_t>(readBuf.potentialSpaceSize()) <= commBufSize);
}
static void
icapLookupDnsResults(const ipcache_addrs *ia, const Dns::LookupDetails &, void *data)
{
Adaptation::Icap::Xaction *xa = static_cast<Adaptation::Icap::Xaction *>(data);
xa->dnsLookupDone(ia);
}
// TODO: obey service-specific, OPTIONS-reported connection limit
void
Adaptation::Icap::Xaction::openConnection()
{
Must(!haveConnection());
Adaptation::Icap::ServiceRep &s = service();
if (!TheConfig.reuse_connections)
disableRetries(); // this will also safely drain pconn pool
@@ -366,123 +359,138 @@
Must(haveConnection());
if (reader != NULL || writer != NULL) {
// restart the timeout before each I/O
// XXX: why does Config.Timeout lacks a write timeout?
// TODO: service bypass status may differ from that of a transaction
typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommTimeoutCbParams> TimeoutDialer;
AsyncCall::Pointer call = JobCallback(93, 5, TimeoutDialer, this, Adaptation::Icap::Xaction::noteCommTimedout);
commSetConnTimeout(connection, TheConfig.io_timeout(service().cfg().bypass), call);
} else {
// clear timeout when there is no I/O
// Do we need a lifetime timeout?
commUnsetConnTimeout(connection);
}
}
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<Adaptation::Icap::Xaction, CommIoCbParams> 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<Adaptation::Icap::Xaction, CommIoCbParams> Dialer;
+ reader = JobCallback(93, 3, Dialer, this, Adaptation::Icap::Xaction::noteCommRead);
+ Comm::Read(connection, reader);
updateTimeout();
}
// comm module read a portion of the ICAP response for us
void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io)
{
Must(reader != NULL);
reader = NULL;
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;
// detect a pconn race condition: eof on the first pconn read
if (!al.icap.bytesRead && retriable()) {
setOutcome(xoRace);
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);
}
void Adaptation::Icap::Xaction::cancelRead()
{
if (reader != NULL) {
Must(haveConnection());
Comm::ReadCancel(connection->fd, reader);
reader = NULL;
}
}
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
Must(mayReadMore());
msg->reset();
return false;
}
readBuf.consume(msg->hdr_sz);
return true;
}
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
{
return commEof;
}
bool Adaptation::Icap::Xaction::doneWriting() const
{
return !writer;
}
bool Adaptation::Icap::Xaction::doneWithIo() const
{
return haveConnection() &&
!connector && !reader && !writer && // fast checks, some redundant
doneReading() && doneWriting();
}
bool Adaptation::Icap::Xaction::haveConnection() const
@@ -513,45 +521,41 @@
debugs(93, 4, HERE << xo);
}
al.icap.outcome = xo;
}
// This 'last chance' method is called before a 'done' transaction is deleted.
// It is wrong to call virtual methods from a destructor. Besides, this call
// indicates that the transaction will terminate as planned.
void Adaptation::Icap::Xaction::swanSong()
{
// kids should sing first and then call the parent method.
if (cs) {
debugs(93,6, HERE << id << " about to notify ConnOpener!");
CallJobHere(93, 3, cs, Comm::ConnOpener, noteAbort);
cs = NULL;
service().noteConnectionFailed("abort");
}
closeConnection(); // TODO: rename because we do not always close
- if (!readBuf.isNull())
- readBuf.clean();
-
- if (commBuf)
- memFreeBuf(commBufSize, commBuf);
+ readBuf.clear();
tellQueryAborted();
maybeLog();
Adaptation::Initiate::swanSong();
}
void Adaptation::Icap::Xaction::tellQueryAborted()
{
if (theInitiator.set()) {
Adaptation::Icap::XactAbortInfo abortInfo(icapRequest, icapReply.getRaw(),
retriable(), repeatable());
Launcher *launcher = dynamic_cast<Launcher*>(theInitiator.get());
// launcher may be nil if initiator is invalid
CallJobHere1(91,5, CbcPointer<Launcher>(launcher),
Launcher, noteXactAbort, abortInfo);
clearInitiator();
}
}
=== modified file 'src/adaptation/icap/Xaction.h'
--- src/adaptation/icap/Xaction.h 2015-01-13 07:25:36 +0000
+++ src/adaptation/icap/Xaction.h 2015-02-28 03:52:10 +0000
@@ -1,39 +1,41 @@
/*
* Copyright (C) 1996-2015 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#ifndef SQUID_ICAPXACTION_H
#define SQUID_ICAPXACTION_H
#include "AccessLogEntry.h"
#include "adaptation/icap/ServiceRep.h"
#include "adaptation/Initiate.h"
#include "comm/forward.h"
#include "CommCalls.h"
#include "HttpReply.h"
#include "ipcache.h"
-#include "MemBuf.h"
+#include "SBuf.h"
+
+class MemBuf;
namespace Adaptation
{
namespace Icap
{
/*
* The ICAP Xaction implements common tasks for ICAP OPTIONS, REQMOD, and
* RESPMOD transactions. It is started by an Initiator. It terminates
* on its own, when done. Transactions communicate with Initiator using
* asynchronous messages because a transaction or Initiator may be gone at
* any time.
*/
// Note: Xaction must be the first parent for object-unaware cbdata to work
class Xaction: public Adaptation::Initiate
{
public:
@@ -110,54 +112,41 @@
/// clear stored error details, if any; used for retries/repeats
virtual void clearError() {}
void dnsLookupDone(const ipcache_addrs *ia);
protected:
// logging
void setOutcome(const XactOutcome &xo);
virtual void finalizeLogInfo();
public:
ServiceRep &service();
private:
void tellQueryAborted();
void maybeLog();
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
bool isRepeatable; ///< can repeat if no or unsatisfactory response
bool ignoreLastWrite;
const char *stopReason;
// active (pending) comm callbacks for the ICAP server connection
AsyncCall::Pointer connector;
AsyncCall::Pointer reader;
AsyncCall::Pointer writer;
AsyncCall::Pointer closer;
AccessLogEntry::Pointer alep; ///< icap.log entry
AccessLogEntry &al; ///< short for *alep
timeval icap_tr_start; /*time when the ICAP transaction was created */
timeval icap_tio_start; /*time when the first ICAP request byte was scheduled for sending*/
timeval icap_tio_finish; /*time when the last byte of the ICAP responsewas received*/
=== modified file 'src/tests/stub_HttpReply.cc'
--- src/tests/stub_HttpReply.cc 2015-01-13 07:25:36 +0000
+++ src/tests/stub_HttpReply.cc 2015-01-25 12:02:45 +0000
@@ -4,29 +4,29 @@
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#include "squid.h"
#include "HttpReply.h"
#define STUB_API "HttpReply.cc"
#include "tests/STUB.h"
HttpReply::HttpReply() : HttpMsg(hoReply), date (0), last_modified (0),
expires (0), surrogate_control (NULL), content_range (NULL), keep_alive (0),
protoPrefix("HTTP/"), do_clean(false), bodySizeMax(-2)
STUB_NOP
HttpReply::~HttpReply() STUB
void HttpReply::setHeaders(Http::StatusCode status, const char *reason, const char *ctype, int64_t clen, time_t lmt, time_t expires_) STUB
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)
void HttpReply::hdrCacheInit() STUB
HttpReply * HttpReply::clone() const STUB_RETVAL(NULL)
bool HttpReply::inheritProperties(const HttpMsg *aMsg) STUB_RETVAL(false)
int64_t HttpReply::bodySize(const HttpRequestMethod&) const STUB_RETVAL(0)
=== modified file 'src/tests/stub_HttpRequest.cc'
--- src/tests/stub_HttpRequest.cc 2015-01-13 07:25:36 +0000
+++ src/tests/stub_HttpRequest.cc 2015-01-25 10:59:54 +0000
@@ -1,30 +1,30 @@
/*
* Copyright (C) 1996-2015 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#include "squid.h"
#include "AccessLogEntry.h"
#include "HttpRequest.h"
#define STUB_API "HttpRequest.cc"
#include "tests/STUB.h"
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)
void HttpRequest::initHTTP(const HttpRequestMethod& aMethod, AnyP::ProtocolType aProtocol, const char *aUrlpath) STUB
bool HttpRequest::parseFirstLine(const char *start, const char *end) STUB_RETVAL(false)
HttpRequest * HttpRequest::clone() const STUB_RETVAL(NULL)
bool HttpRequest::inheritProperties(const HttpMsg *aMsg) STUB_RETVAL(false)
int64_t HttpRequest::getRangeOffsetLimit() STUB_RETVAL(0)
const char *HttpRequest::storeId() STUB_RETVAL(".")
=== modified file 'src/tests/testHttpReply.cc'
--- src/tests/testHttpReply.cc 2015-01-13 07:25:36 +0000
+++ src/tests/testHttpReply.cc 2015-01-25 14:39:02 +0000
@@ -33,172 +33,172 @@
void
testHttpReply::setUp()
{
Mem::Init();
httpHeaderInitModule();
}
void
testHttpReply::testSanityCheckFirstLine()
{
MemBuf input;
HttpReply engine;
Http::StatusCode error = Http::scNone;
size_t hdr_len;
input.init();
// 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;
#if TODO // these cases are only checked after parse...
// 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;
#endif
// 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;
/* NP: the engine saves details about the protocol. even when being reset :( */
engine.protoPrefix="HTTP/";
engine.reset();
// 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;
// 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;
// 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;
// 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;
// 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;
// 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;
}
=== modified file 'src/tests/testHttpRequest.cc'
--- src/tests/testHttpRequest.cc 2015-01-13 07:25:36 +0000
+++ src/tests/testHttpRequest.cc 2015-01-25 12:17:15 +0000
@@ -5,41 +5,41 @@
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#include "squid.h"
#include <cppunit/TestAssert.h>
#include "HttpHeader.h"
#include "HttpRequest.h"
#include "mime_header.h"
#include "testHttpRequest.h"
#include "unitTestMain.h"
CPPUNIT_TEST_SUITE_REGISTRATION( testHttpRequest );
/** wrapper for testing HttpRequest object private and protected functions */
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 */
void
testHttpRequest::setUp()
{
Mem::Init();
httpHeaderInitModule();
}
/*
* Test creating an HttpRequest object from a Url and method
*/
void
testHttpRequest::testCreateFromUrlAndMethod()
{
/* vanilla url */
unsigned short expected_port;
char * url = xstrdup("http://foo:90/bar");
@@ -147,65 +147,65 @@
CPPUNIT_ASSERT(aRequest->method == Http::METHOD_GET);
CPPUNIT_ASSERT_EQUAL(String("[2000:800::45]"), String(aRequest->GetHost()));
CPPUNIT_ASSERT_EQUAL(String("/foo"), aRequest->urlpath);
CPPUNIT_ASSERT_EQUAL(AnyP::PROTO_HTTP, static_cast<AnyP::ProtocolType>(aRequest->url.getScheme()));
CPPUNIT_ASSERT_EQUAL(String("http://2000:800::45/foo"), String(url));
xfree(url);
}
void
testHttpRequest::testSanityCheckStartLine()
{
MemBuf input;
PrivateHttpRequest engine;
Http::StatusCode error = Http::scNone;
size_t hdr_len;
input.init();
// 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;
// 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;
// TODO no method
// TODO binary code in method
// TODO no URL
// TODO no status (okay)
// TODO non-HTTP protocol
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;
}
=== modified file 'src/tunnel.cc'
--- src/tunnel.cc 2015-02-26 12:12:11 +0000
+++ src/tunnel.cc 2015-02-27 11:55:17 +0000
@@ -396,41 +396,42 @@
}
/// Parses [possibly incomplete] CONNECT response and reacts to it.
/// If the tunnel is being closed or more response data is needed, returns false.
/// Otherwise, the caller should handle the remaining read data, if any.
void
TunnelStateData::handleConnectResponse(const size_t chunkSize)
{
assert(waitingForConnectResponse());
// Ideally, client and server should use MemBuf or better, but current code
// never accumulates more than one read when shoveling data (XXX) so it does
// not need to deal with MemBuf complexity. To keep it simple, we use a
// dedicated MemBuf for accumulating CONNECT responses. TODO: When shoveling
// is optimized, reuse server.buf for CONNEC response accumulation instead.
/* mimic the basic parts of HttpStateData::processReplyHeader() */
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");
return;
}
// need more data
assert(!eof);
assert(!parseErr);
if (!connectRespBuf->hasSpace()) {
server.logicError("huge CONNECT response from peer");
return;
}
// keep reading
readConnectResponse();
return;
}
More information about the squid-dev
mailing list