From: Christos Tsantilas Date: Mon, 25 Jul 2016 10:22:54 +0000 (+0300) Subject: Certificate Validator buffer-overflow crashes Squid X-Git-Tag: SQUID_4_0_13~17 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ddc77a2e9f0a68604f05708796071487f4dfe3ea;p=thirdparty%2Fsquid.git Certificate Validator buffer-overflow crashes Squid Certain server certificates with a large chain and/or large certificates (i.e. due to excessive amount of SAN entries) are producing helper requests and replies which are larger than the 32KB limit set in src/helper.cc. The major problem for squid is that the helper response should fit in the helper read buffer. Currently squid starts with 4K request buffer and if required may increase the buffer size up to 32K. This patch: - Uses a constant-size read buffer for helpers and accumulates the helper response to Helper::Reply object. - Changes the HelperServerBase::requests list to hold list of the new Helper::Xaction class which holds pairs of Helper::Request and Helper::Reply objects - Modifies the Helper::Reply class to accumulate data and restricts the required memory allocations This is a Measurement Factory project --- diff --git a/src/MemBuf.cc b/src/MemBuf.cc index 4e2b9ef5c2..8e68de502d 100644 --- a/src/MemBuf.cc +++ b/src/MemBuf.cc @@ -154,7 +154,7 @@ MemBuf::reset() * Unfortunate hack to test if the buffer has been Init()ialized */ int -MemBuf::isNull() +MemBuf::isNull() const { if (!buf && !max_capacity && !capacity && !size) return 1; /* is null (not initialized) */ diff --git a/src/MemBuf.h b/src/MemBuf.h index 1372fb46e1..00716f05d1 100644 --- a/src/MemBuf.h +++ b/src/MemBuf.h @@ -99,7 +99,7 @@ public: void reset(); /** unfirtunate hack to test if the buffer has been Init()ialized */ - int isNull(); + int isNull() const; /** * freezes the object! and returns function to clear it up. diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 638202d7ca..e26ec98a3f 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -885,8 +885,7 @@ clientRedirectAccessCheckDone(allow_t answer, void *data) if (answer == ACCESS_ALLOWED) redirectStart(http, clientRedirectDoneWrapper, context); else { - Helper::Reply nilReply; - nilReply.result = Helper::Error; + Helper::Reply const nilReply(Helper::Error); context->clientRedirectDone(nilReply); } } @@ -918,8 +917,7 @@ clientStoreIdAccessCheckDone(allow_t answer, void *data) storeIdStart(http, clientStoreIdDoneWrapper, context); else { debugs(85, 3, "access denied expected ERR reply handling: " << answer); - Helper::Reply nilReply; - nilReply.result = Helper::Error; + Helper::Reply const nilReply(Helper::Error); context->clientStoreIdDone(nilReply); } } diff --git a/src/helper.cc b/src/helper.cc index cab6fa6a80..95e8da0527 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -37,31 +37,23 @@ /// The maximum allowed request retries. #define MAX_RETRIES 2 -/** Initial Squid input buffer size. Helper responses may exceed this, and - * Squid will grow the input buffer as needed, up to ReadBufMaxSize. - */ -const size_t ReadBufMinSize(4*1024); +/// Helpers input buffer size. +const size_t ReadBufSize(32*1024); -/** Maximum safe size of a helper-to-Squid response message plus one. - * Squid will warn and close the stream if a helper sends a too-big response. - * ssl_crtd helper is known to produce responses of at least 10KB in size. - * Some undocumented helpers are known to produce responses exceeding 8KB. - */ -const size_t ReadBufMaxSize(32*1024); static IOCB helperHandleRead; static IOCB helperStatefulHandleRead; static void helperServerFree(helper_server *srv); static void helperStatefulServerFree(helper_stateful_server *srv); -static void Enqueue(helper * hlp, Helper::Request *); +static void Enqueue(helper * hlp, Helper::Xaction *); static helper_server *GetFirstAvailable(helper * hlp); static helper_stateful_server *StatefulGetFirstAvailable(statefulhelper * hlp); -static void helperDispatch(helper_server * srv, Helper::Request * r); -static void helperStatefulDispatch(helper_stateful_server * srv, Helper::Request * r); +static void helperDispatch(helper_server * srv, Helper::Xaction * r); +static void helperStatefulDispatch(helper_stateful_server * srv, Helper::Xaction * r); static void helperKickQueue(helper * hlp); static void helperStatefulKickQueue(statefulhelper * hlp); static void helperStatefulServerDone(helper_stateful_server * srv); -static void StatefulEnqueue(statefulhelper * hlp, Helper::Request * r); +static void StatefulEnqueue(statefulhelper * hlp, Helper::Xaction * r); CBDATA_CLASS_INIT(helper); CBDATA_CLASS_INIT(helper_server); @@ -212,10 +204,11 @@ helperOpenServers(helper * hlp) srv->readPipe->fd = rfd; srv->writePipe = new Comm::Connection; srv->writePipe->fd = wfd; - srv->rbuf = (char *)memAllocBuf(ReadBufMinSize, &srv->rbuf_sz); + srv->rbuf = (char *)memAllocBuf(ReadBufSize, &srv->rbuf_sz); srv->wqueue = new MemBuf; srv->roffset = 0; srv->nextRequestId = 0; + srv->replyXaction = NULL; srv->parent = cbdataReference(hlp); dlinkAddTail(srv, &srv->link, &hlp->servers); @@ -338,7 +331,7 @@ helperStatefulOpenServers(statefulhelper * hlp) srv->readPipe->fd = rfd; srv->writePipe = new Comm::Connection; srv->writePipe->fd = wfd; - srv->rbuf = (char *)memAllocBuf(ReadBufMinSize, &srv->rbuf_sz); + srv->rbuf = (char *)memAllocBuf(ReadBufSize, &srv->rbuf_sz); srv->roffset = 0; srv->parent = cbdataReference(hlp); @@ -377,7 +370,7 @@ helperStatefulOpenServers(statefulhelper * hlp) } void -helper::submitRequest(Helper::Request *r) +helper::submitRequest(Helper::Xaction *r) { helper_server *srv; @@ -399,7 +392,7 @@ helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data) { if (hlp == NULL) { debugs(84, 3, "helperSubmit: hlp == NULL"); - Helper::Reply nilReply; + Helper::Reply const nilReply(Helper::Unknown); callback(data, nilReply); return; } @@ -443,7 +436,7 @@ helper::trySubmit(const char *buf, HLPCB * callback, void *data) void helper::submit(const char *buf, HLPCB * callback, void *data) { - Helper::Request *r = new Helper::Request(callback, data, buf); + Helper::Xaction *r = new Helper::Xaction(callback, data, buf); submitRequest(r); debugs(84, DBG_DATA, Raw("buf", buf, strlen(buf))); } @@ -454,7 +447,7 @@ helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, vo { if (hlp == NULL) { debugs(84, 3, "helperStatefulSubmit: hlp == NULL"); - Helper::Reply nilReply; + Helper::Reply const nilReply(Helper::Unknown); callback(data, nilReply); return; } @@ -464,7 +457,7 @@ helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, vo void statefulhelper::submit(const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver) { - Helper::Request *r = new Helper::Request(callback, data, buf); + Helper::Xaction *r = new Helper::Xaction(callback, data, buf); if ((buf != NULL) && lastserver) { debugs(84, 5, "StatefulSubmit with lastserver " << lastserver); @@ -481,7 +474,7 @@ void statefulhelper::submit(const char *buf, HLPCB * callback, void *data, helpe StatefulEnqueue(this, r); } - debugs(84, DBG_DATA, "placeholder: '" << r->placeholder << + debugs(84, DBG_DATA, "placeholder: '" << r->request.placeholder << "', " << Raw("buf", buf, (!buf?0:strlen(buf)))); if (!queueFull()) { @@ -548,8 +541,8 @@ helper::packStatsInto(Packable *p, const char *label) const for (dlink_node *link = servers.head; link; link = link->next) { HelperServerBase *srv = static_cast(link->data); assert(srv); - Helper::Request *request = srv->requests.empty() ? NULL : srv->requests.front(); - double tt = 0.001 * (request ? tvSubMsec(request->dispatch_time, current_time) : tvSubMsec(srv->dispatch_time, srv->answer_time)); + Helper::Xaction *xaction = srv->requests.empty() ? NULL : srv->requests.front(); + double tt = 0.001 * (xaction ? tvSubMsec(xaction->request.dispatch_time, current_time) : tvSubMsec(srv->dispatch_time, srv->answer_time)); p->appendf("%7u\t%7d\t%7d\t%11" PRIu64 "\t%11" PRIu64 "\t%11" PRIu64 "\t%c%c%c%c%c%c\t%7.3f\t%7d\t%s\n", srv->index.value, srv->readPipe->fd, @@ -562,10 +555,10 @@ helper::packStatsInto(Packable *p, const char *label) const srv->flags.closing ? 'C' : ' ', srv->flags.reserved ? 'R' : ' ', srv->flags.shutdown ? 'S' : ' ', - request && request->placeholder ? 'P' : ' ', + xaction && xaction->request.placeholder ? 'P' : ' ', tt < 0.0 ? 0.0 : tt, (int) srv->roffset, - request ? Format::QuoteMimeBlob(request->buf) : "(none)"); + xaction ? Format::QuoteMimeBlob(xaction->request.buf) : "(none)"); } p->append("\nFlags key:\n" @@ -727,13 +720,13 @@ helperServerFree(helper_server *srv) while (!srv->requests.empty()) { // XXX: re-schedule these on another helper? - Helper::Request *r = srv->requests.front(); + Helper::Xaction *r = srv->requests.front(); srv->requests.pop_front(); void *cbdata; - if (cbdataReferenceValidDone(r->data, &cbdata)) { - Helper::Reply nilReply; - r->callback(cbdata, nilReply); + if (cbdataReferenceValidDone(r->request.data, &cbdata)) { + r->reply.result = Helper::Unknown; + r->request.callback(cbdata, r->reply); } delete r; @@ -792,13 +785,13 @@ helperStatefulServerFree(helper_stateful_server *srv) while (!srv->requests.empty()) { // XXX: re-schedule these on another helper? - Helper::Request *r = srv->requests.front(); + Helper::Xaction *r = srv->requests.front(); srv->requests.pop_front(); void *cbdata; - if (cbdataReferenceValidDone(r->data, &cbdata)) { - Helper::Reply nilReply; - r->callback(cbdata, nilReply); + if (cbdataReferenceValidDone(r->request.data, &cbdata)) { + r->reply.result = Helper::Unknown; + r->request.callback(cbdata, r->reply); } delete r; @@ -812,39 +805,57 @@ helperStatefulServerFree(helper_stateful_server *srv) delete srv; } -/// Calls back with a pointer to the buffer with the helper output -static void -helperReturnBuffer(int request_number, helper_server * srv, helper * hlp, char * msg, char * msg_end) +Helper::Xaction * +helper_server::popRequest(int request_number) { - Helper::Request *r = NULL; + Helper::Xaction *r = nullptr; helper_server::RequestIndex::iterator it; - if (hlp->childs.concurrency) { + if (parent->childs.concurrency) { // If concurency supported retrieve request from ID - it = srv->requestsIndex.find(request_number); - if (it != srv->requestsIndex.end()) { + it = requestsIndex.find(request_number); + if (it != requestsIndex.end()) { r = *(it->second); - srv->requests.erase(it->second); - srv->requestsIndex.erase(it); + requests.erase(it->second); + requestsIndex.erase(it); } - } else if(!srv->requests.empty()) { + } else if(!requests.empty()) { // Else get the first request from queue, if any - r = srv->requests.front(); - srv->requests.pop_front(); + r = requests.front(); + requests.pop_front(); } - if (r) { - HLPCB *callback = r->callback; - r->callback = NULL; + return r; +} + +/// Calls back with a pointer to the buffer with the helper output +static void +helperReturnBuffer(helper_server * srv, helper * hlp, char * msg, size_t msgSize, char * msgEnd) +{ + if (Helper::Xaction *r = srv->replyXaction) { + const bool hasSpace = r->reply.accumulate(msg, msgSize); + if (!hasSpace) { + debugs(84, DBG_IMPORTANT, "ERROR: Disconnecting from a " << + "helper that overflowed " << srv->rbuf_sz << "-byte " << + "Squid input buffer: " << hlp->id_name << " #" << srv->index); + srv->closePipesSafely(hlp->id_name); + return; + } + + if (!msgEnd) + return; // We are waiting for more data. + + HLPCB *callback = r->request.callback; + r->request.callback = nullptr; - void *cbdata = NULL; bool retry = false; - if (cbdataReferenceValidDone(r->data, &cbdata)) { - Helper::Reply response(msg, (msg_end-msg)); - if (response.result == Helper::BrokenHelper && r->retries < MAX_RETRIES) { - debugs(84, DBG_IMPORTANT, "ERROR: helper: " << response << ", attempt #" << (r->retries + 1) << " of 2"); + void *cbdata = nullptr; + if (cbdataReferenceValidDone(r->request.data, &cbdata)) { + r->reply.finalize(); + if (r->reply.result == Helper::BrokenHelper && r->request.retries < MAX_RETRIES) { + debugs(84, DBG_IMPORTANT, "ERROR: helper: " << r->reply << ", attempt #" << (r->request.retries + 1) << " of 2"); retry = true; } else - callback(cbdata, response); + callback(cbdata, r->reply); } -- srv->stats.pending; @@ -854,24 +865,20 @@ helperReturnBuffer(int request_number, helper_server * srv, helper * hlp, char * srv->answer_time = current_time; - srv->dispatch_time = r->dispatch_time; + srv->dispatch_time = r->request.dispatch_time; hlp->stats.avg_svc_time = Math::intAverage(hlp->stats.avg_svc_time, - tvSubMsec(r->dispatch_time, current_time), + tvSubMsec(r->request.dispatch_time, current_time), hlp->stats.replies, REDIRECT_AV_FACTOR); + // release or re-submit parsedRequestXaction object + srv->replyXaction = nullptr; if (retry) { - ++r->retries; + ++r->request.retries; hlp->submitRequest(r); } else delete r; - } else if (srv->stats.timedout) { - debugs(84, 3, "Timedout reply received for request-ID: " << request_number << " , ignore"); - } else { - debugs(84, DBG_IMPORTANT, "helperHandleRead: unexpected reply on channel " << - request_number << " from " << hlp->id_name << " #" << srv->index << - " '" << srv->rbuf << "'"); } if (hlp->timeout && hlp->childs.concurrency) @@ -888,7 +895,6 @@ helperReturnBuffer(int request_number, helper_server * srv, helper * hlp, char * static void helperHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len, Comm::Flag flag, int, void *data) { - char *t = NULL; helper_server *srv = (helper_server *)data; helper *hlp = srv->parent; assert(cbdataReferenceValid(data)); @@ -922,57 +928,75 @@ helperHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len, Comm:: srv->rbuf[0] = '\0'; } - while ((t = strchr(srv->rbuf, hlp->eom))) { - /* end of reply found */ - char *msg = srv->rbuf; - int i = 0; - int skip = 1; - debugs(84, 3, "helperHandleRead: end of reply found"); - - if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n') { - *t = '\0'; - // rewind to the \r octet which is the real terminal now - // and remember that we have to skip forward 2 places now. - skip = 2; - --t; + bool needsMore = false; + char *msg = srv->rbuf; + while (*msg && !needsMore) { + int skip = 0; + char *eom = strchr(msg, hlp->eom); + if (eom) { + skip = 1; + debugs(84, 3, "helperHandleRead: end of reply found"); + if (eom > msg && eom[-1] == '\r' && hlp->eom == '\n') { + *eom = '\0'; + // rewind to the \r octet which is the real terminal now + // and remember that we have to skip forward 2 places now. + skip = 2; + --eom; + } + *eom = '\0'; } - *t = '\0'; - - if (hlp->childs.concurrency) { - i = strtol(msg, &msg, 10); - - while (*msg && xisspace(*msg)) - ++msg; - } + if (!srv->replyXaction) { + int i = 0; + if (hlp->childs.concurrency) { + char *e = NULL; + i = strtol(msg, &e, 10); + // Do we need to check for e == msg? Means wrong response from helper. + // Will be droped as "unexpected reply on channel 0" + needsMore = !(xisspace(*e) || (eom && e == eom)); + if (!needsMore) { + msg = e; + while (*msg && xisspace(*msg)) + ++msg; + } // else not enough data to compute request number + } + if (!(srv->replyXaction = srv->popRequest(i))) { + if (srv->stats.timedout) { + debugs(84, 3, "Timedout reply received for request-ID: " << i << " , ignore"); + } else { + debugs(84, DBG_IMPORTANT, "helperHandleRead: unexpected reply on channel " << + i << " from " << hlp->id_name << " #" << srv->index << + " '" << srv->rbuf << "'"); + } + } + } // else we need to just append reply data to the current Xaction + + if (!needsMore) { + size_t msgSize = eom ? eom - msg : (srv->roffset - (msg - srv->rbuf)); + assert(msgSize <= srv->rbuf_sz); + helperReturnBuffer(srv, hlp, msg, msgSize, eom); + msg += msgSize + skip; + assert(static_cast(msg - srv->rbuf) <= srv->rbuf_sz); + } else + assert(skip == 0 && eom == NULL); + } - helperReturnBuffer(i, srv, hlp, msg, t); - srv->roffset -= (t - srv->rbuf) + skip; - memmove(srv->rbuf, t + skip, srv->roffset); + if (needsMore) { + size_t msgSize = (srv->roffset - (msg - srv->rbuf)); + assert(msgSize <= srv->rbuf_sz); + memmove(srv->rbuf, msg, msgSize); + srv->roffset = msgSize; srv->rbuf[srv->roffset] = '\0'; + } else { + // All of the responses parsed and msg points at the end of read data + assert(static_cast(msg - srv->rbuf) == srv->roffset); + srv->roffset = 0; } if (Comm::IsConnOpen(srv->readPipe) && !fd_table[srv->readPipe->fd].closing()) { int spaceSize = srv->rbuf_sz - srv->roffset - 1; assert(spaceSize >= 0); - // grow the input buffer if needed and possible - if (!spaceSize && srv->rbuf_sz + 4096 <= ReadBufMaxSize) { - srv->rbuf = (char *)memReallocBuf(srv->rbuf, srv->rbuf_sz + 4096, &srv->rbuf_sz); - debugs(84, 3, HERE << "Grew read buffer to " << srv->rbuf_sz); - spaceSize = srv->rbuf_sz - srv->roffset - 1; - assert(spaceSize >= 0); - } - - // quit reading if there is no space left - if (!spaceSize) { - debugs(84, DBG_IMPORTANT, "ERROR: Disconnecting from a " << - "helper that overflowed " << srv->rbuf_sz << "-byte " << - "Squid input buffer: " << hlp->id_name << " #" << srv->index); - srv->closePipesSafely(hlp->id_name); - return; - } - AsyncCall::Pointer call = commCbCall(5,4, "helperHandleRead", CommIoCbPtrFun(helperHandleRead, srv)); comm_read(srv->readPipe, srv->rbuf + srv->roffset, spaceSize, call); @@ -1005,7 +1029,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len srv->roffset += len; srv->rbuf[srv->roffset] = '\0'; - Helper::Request *r = srv->requests.front(); + Helper::Xaction *r = srv->requests.front(); debugs(84, DBG_DATA, Raw("accumulated", srv->rbuf, srv->roffset)); if (r == NULL) { @@ -1018,40 +1042,46 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len } if ((t = strchr(srv->rbuf, hlp->eom))) { - /* end of reply found */ - srv->requests.pop_front(); // we already have it in 'r' - int called = 1; - int skip = 1; debugs(84, 3, "helperStatefulHandleRead: end of reply found"); if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n') { *t = '\0'; // rewind to the \r octet which is the real terminal now - // and remember that we have to skip forward 2 places now. - skip = 2; --t; } *t = '\0'; + } + + if (!r->reply.accumulate(srv->rbuf, t ? (t - srv->rbuf) : srv->roffset)) { + debugs(84, DBG_IMPORTANT, "ERROR: Disconnecting from a " << + "helper that overflowed " << srv->rbuf_sz << "-byte " << + "Squid input buffer: " << hlp->id_name << " #" << srv->index); + srv->closePipesSafely(hlp->id_name); + return; + } + /** + * BUG: the below assumes that only one response per read() was received and discards any octets remaining. + * Doing this prohibits concurrency support with multiple replies per read(). + * TODO: check that read() setup on these buffers pays attention to roffest!=0 + * TODO: check that replies bigger than the buffer are discarded and do not to affect future replies + */ + srv->roffset = 0; + + if (t) { + /* end of reply found */ + srv->requests.pop_front(); // we already have it in 'r' + int called = 1; - if (r && cbdataReferenceValid(r->data)) { - Helper::Reply res(srv->rbuf, (t - srv->rbuf)); - res.whichServer = srv; - r->callback(r->data, res); + if (r && cbdataReferenceValid(r->request.data)) { + r->reply.finalize(); + r->reply.whichServer = srv; + r->request.callback(r->request.data, r->reply); } else { debugs(84, DBG_IMPORTANT, "StatefulHandleRead: no callback data registered"); called = 0; } - // only skip off the \0's _after_ passing its location in Helper::Reply above - t += skip; - - /** - * BUG: the below assumes that only one response per read() was received and discards any octets remaining. - * Doing this prohibits concurrency support with multiple replies per read(). - * TODO: check that read() setup on these buffers pays attention to roffest!=0 - * TODO: check that replies bigger than the buffer are discarded and do not to affect future replies - */ - srv->roffset = 0; + delete r; -- srv->stats.pending; @@ -1071,35 +1101,17 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len } if (Comm::IsConnOpen(srv->readPipe) && !fd_table[srv->readPipe->fd].closing()) { - int spaceSize = srv->rbuf_sz - srv->roffset - 1; - assert(spaceSize >= 0); - - // grow the input buffer if needed and possible - if (!spaceSize && srv->rbuf_sz + 4096 <= ReadBufMaxSize) { - srv->rbuf = (char *)memReallocBuf(srv->rbuf, srv->rbuf_sz + 4096, &srv->rbuf_sz); - debugs(84, 3, HERE << "Grew read buffer to " << srv->rbuf_sz); - spaceSize = srv->rbuf_sz - srv->roffset - 1; - assert(spaceSize >= 0); - } - - // quit reading if there is no space left - if (!spaceSize) { - debugs(84, DBG_IMPORTANT, "ERROR: Disconnecting from a " << - "helper that overflowed " << srv->rbuf_sz << "-byte " << - "Squid input buffer: " << hlp->id_name << " #" << srv->index); - srv->closePipesSafely(hlp->id_name); - return; - } + int spaceSize = srv->rbuf_sz - 1; AsyncCall::Pointer call = commCbCall(5,4, "helperStatefulHandleRead", CommIoCbPtrFun(helperStatefulHandleRead, srv)); - comm_read(srv->readPipe, srv->rbuf + srv->roffset, spaceSize, call); + comm_read(srv->readPipe, srv->rbuf, spaceSize, call); } } /// Handles a request when all running helpers, if any, are busy. static void -Enqueue(helper * hlp, Helper::Request * r) +Enqueue(helper * hlp, Helper::Xaction * r) { hlp->queue.push(r); ++ hlp->stats.queue_size; @@ -1128,7 +1140,7 @@ Enqueue(helper * hlp, Helper::Request * r) } static void -StatefulEnqueue(statefulhelper * hlp, Helper::Request * r) +StatefulEnqueue(statefulhelper * hlp, Helper::Xaction * r) { hlp->queue.push(r); ++ hlp->stats.queue_size; @@ -1156,7 +1168,7 @@ StatefulEnqueue(statefulhelper * hlp, Helper::Request * r) debugs(84, DBG_CRITICAL, "WARNING: Consider increasing the number of " << hlp->id_name << " processes in your config file."); } -Helper::Request * +Helper::Xaction * helper::nextRequest() { if (queue.empty()) @@ -1272,20 +1284,20 @@ helperDispatchWriteDone(const Comm::ConnectionPointer &, char *, size_t, Comm::F } static void -helperDispatch(helper_server * srv, Helper::Request * r) +helperDispatch(helper_server * srv, Helper::Xaction * r) { helper *hlp = srv->parent; const uint64_t reqId = ++srv->nextRequestId; - if (!cbdataReferenceValid(r->data)) { + if (!cbdataReferenceValid(r->request.data)) { debugs(84, DBG_IMPORTANT, "helperDispatch: invalid callback data"); delete r; return; } - r->Id = reqId; + r->request.Id = reqId; helper_server::Requests::iterator it = srv->requests.insert(srv->requests.end(), r); - r->dispatch_time = current_time; + r->request.dispatch_time = current_time; if (srv->wqueue->isNull()) srv->wqueue->init(); @@ -1293,9 +1305,9 @@ helperDispatch(helper_server * srv, Helper::Request * r) if (hlp->childs.concurrency) { srv->requestsIndex.insert(helper_server::RequestIndex::value_type(reqId, it)); assert(srv->requestsIndex.size() == srv->requests.size()); - srv->wqueue->appendf("%" PRIu64 " %s", reqId, r->buf); + srv->wqueue->appendf("%" PRIu64 " %s", reqId, r->request.buf); } else - srv->wqueue->append(r->buf, strlen(r->buf)); + srv->wqueue->append(r->request.buf, strlen(r->request.buf)); if (!srv->flags.writing) { assert(NULL == srv->writebuf); @@ -1307,7 +1319,7 @@ helperDispatch(helper_server * srv, Helper::Request * r) Comm::Write(srv->writePipe, srv->writebuf->content(), srv->writebuf->contentSize(), call, NULL); } - debugs(84, 5, "helperDispatch: Request sent to " << hlp->id_name << " #" << srv->index << ", " << strlen(r->buf) << " bytes"); + debugs(84, 5, "helperDispatch: Request sent to " << hlp->id_name << " #" << srv->index << ", " << strlen(r->request.buf) << " bytes"); ++ srv->stats.uses; ++ srv->stats.pending; @@ -1319,11 +1331,11 @@ helperStatefulDispatchWriteDone(const Comm::ConnectionPointer &, char *, size_t, {} static void -helperStatefulDispatch(helper_stateful_server * srv, Helper::Request * r) +helperStatefulDispatch(helper_stateful_server * srv, Helper::Xaction * r) { statefulhelper *hlp = srv->parent; - if (!cbdataReferenceValid(r->data)) { + if (!cbdataReferenceValid(r->request.data)) { debugs(84, DBG_IMPORTANT, "helperStatefulDispatch: invalid callback data"); delete r; helperStatefulReleaseServer(srv); @@ -1332,13 +1344,13 @@ helperStatefulDispatch(helper_stateful_server * srv, Helper::Request * r) debugs(84, 9, "helperStatefulDispatch busying helper " << hlp->id_name << " #" << srv->index); - if (r->placeholder == 1) { + if (r->request.placeholder == 1) { /* a callback is needed before this request can _use_ a helper. */ /* we don't care about releasing this helper. The request NEVER * gets to the helper. So we throw away the return code */ - Helper::Reply nilReply; - nilReply.whichServer = srv; - r->callback(r->data, nilReply); + r->reply.result = Helper::Unknown; + r->reply.whichServer = srv; + r->request.callback(r->request.data, r->reply); /* throw away the placeholder */ delete r; /* and push the queue. Note that the callback may have submitted a new @@ -1355,10 +1367,10 @@ helperStatefulDispatch(helper_stateful_server * srv, Helper::Request * r) srv->dispatch_time = current_time; AsyncCall::Pointer call = commCbCall(5,5, "helperStatefulDispatchWriteDone", CommIoCbPtrFun(helperStatefulDispatchWriteDone, hlp)); - Comm::Write(srv->writePipe, r->buf, strlen(r->buf), call, NULL); + Comm::Write(srv->writePipe, r->request.buf, strlen(r->request.buf), call, NULL); debugs(84, 5, "helperStatefulDispatch: Request sent to " << hlp->id_name << " #" << srv->index << ", " << - (int) strlen(r->buf) << " bytes"); + (int) strlen(r->request.buf) << " bytes"); ++ srv->stats.uses; ++ srv->stats.pending; @@ -1368,7 +1380,7 @@ helperStatefulDispatch(helper_stateful_server * srv, Helper::Request * r) static void helperKickQueue(helper * hlp) { - Helper::Request *r; + Helper::Xaction *r; helper_server *srv; while ((srv = GetFirstAvailable(hlp)) && (r = hlp->nextRequest())) @@ -1378,7 +1390,7 @@ helperKickQueue(helper * hlp) static void helperStatefulKickQueue(statefulhelper * hlp) { - Helper::Request *r; + Helper::Xaction *r; helper_stateful_server *srv; while ((srv = StatefulGetFirstAvailable(hlp)) && (r = hlp->nextRequest())) @@ -1400,29 +1412,32 @@ void helper_server::checkForTimedOutRequests(bool const retry) { assert(parent->childs.concurrency); - while(!requests.empty() && requests.front()->timedOut(parent->timeout)) { - Helper::Request *r = requests.front(); + while(!requests.empty() && requests.front()->request.timedOut(parent->timeout)) { + Helper::Xaction *r = requests.front(); RequestIndex::iterator it; - it = requestsIndex.find(r->Id); + it = requestsIndex.find(r->request.Id); assert(it != requestsIndex.end()); requestsIndex.erase(it); requests.pop_front(); - debugs(84, 2, "Request " << r->Id << " timed-out, remove it from queue"); + debugs(84, 2, "Request " << r->request.Id << " timed-out, remove it from queue"); void *cbdata; bool retried = false; - if (retry && r->retries < MAX_RETRIES && cbdataReferenceValid(r->data)) { - debugs(84, 2, "Retry request " << r->Id); - ++r->retries; + if (retry && r->request.retries < MAX_RETRIES && cbdataReferenceValid(r->request.data)) { + debugs(84, 2, "Retry request " << r->request.Id); + ++r->request.retries; parent->submitRequest(r); retried = true; - } else if (cbdataReferenceValidDone(r->data, &cbdata)) { + } else if (cbdataReferenceValidDone(r->request.data, &cbdata)) { if (!parent->onTimedOutResponse.isEmpty()) { - // Helper::Reply needs a non const buffer - char *replyMsg = xstrdup(parent->onTimedOutResponse.c_str()); - r->callback(cbdata, Helper::Reply(replyMsg, strlen(replyMsg))); - xfree(replyMsg); - } else - r->callback(cbdata, Helper::Reply(Helper::TimedOut)); + if (r->reply.accumulate(parent->onTimedOutResponse.rawContent(), parent->onTimedOutResponse.length())) + r->reply.finalize(); + else + r->reply.result = Helper::TimedOut; + r->request.callback(cbdata, r->reply); + } else { + r->reply.result = Helper::TimedOut; + r->request.callback(cbdata, r->reply); + } } --stats.pending; ++stats.timedout; @@ -1447,7 +1462,7 @@ helper_server::requestTimeout(const CommTimeoutCbParams &io) AsyncCall::Pointer timeoutCall = commCbCall(84, 4, "helper_server::requestTimeout", CommTimeoutCbPtrFun(helper_server::requestTimeout, srv)); - const int timeSpent = srv->requests.empty() ? 0 : (squid_curtime - srv->requests.front()->dispatch_time.tv_sec); + const int timeSpent = srv->requests.empty() ? 0 : (squid_curtime - srv->requests.front()->request.dispatch_time.tv_sec); const int timeLeft = max(1, (static_cast(srv->parent->timeout) - timeSpent)); commSetConnTimeout(io.conn, timeLeft, timeoutCall); diff --git a/src/helper.h b/src/helper.h index 63eea4f590..d655811f28 100644 --- a/src/helper.h +++ b/src/helper.h @@ -18,6 +18,8 @@ #include "dlink.h" #include "helper/ChildConfig.h" #include "helper/forward.h" +#include "helper/Request.h" +#include "helper/Reply.h" #include "ip/Address.h" #include "sbuf/SBuf.h" @@ -28,6 +30,18 @@ class Packable; class wordlist; +namespace Helper +{ +/// Holds the required data to serve a helper request. +class Xaction { + MEMPROXY_CLASS(Helper::Xaction); +public: + Xaction(HLPCB *c, void *d, const char *b): request(c, d, b) {} + Helper::Request request; + Helper::Reply reply; +}; +} + /** * Managers a set of individual helper processes with a common queue of requests. * @@ -67,14 +81,14 @@ public: bool queueFull() const; /// \returns next request in the queue, or nil. - Helper::Request *nextRequest(); + Helper::Xaction *nextRequest(); ///< If not full, submit request. Otherwise, either kill Squid or return false. bool trySubmit(const char *buf, HLPCB * callback, void *data); /// Submits a request to the helper or add it to the queue if none of /// the servers is available. - void submitRequest(Helper::Request *r); + void submitRequest(Helper::Xaction *r); /// Dump some stats about the helper state to a Packable object void packStatsInto(Packable *p, const char *label = NULL) const; @@ -82,7 +96,7 @@ public: public: wordlist *cmdline; dlink_list servers; - std::queue queue; + std::queue queue; const char *id_name; Helper::ChildConfig childs; ///< Configuration settings for number running. int ipc_type; @@ -173,7 +187,7 @@ public: bool reserved; } flags; - typedef std::list Requests; + typedef std::list Requests; Requests requests; ///< requests in order of submission/expiration struct { @@ -201,10 +215,22 @@ public: helper *parent; + /// The helper request Xaction object for the current reply . + /// A helper reply may be distributed to more than one of the retrieved + /// packets from helper. This member stores the Xaction object as long as + /// the end-of-message for current reply is not retrieved. + Helper::Xaction *replyXaction; + // STL says storing std::list iterators is safe when changing the list typedef std::map RequestIndex; RequestIndex requestsIndex; ///< maps request IDs to requests + /// Search in queue for the request with requestId, return the related + /// Xaction object and remove it from queue. + /// If concurrency is disabled then the requestId is ignored and the + /// Xaction of the next request in queue is retrieved. + Helper::Xaction *popRequest(int requestId); + /// Run over the active requests lists and forces a retry, or timedout reply /// or the configured "on timeout response" for timedout requests. void checkForTimedOutRequests(bool const retry); diff --git a/src/helper/Reply.cc b/src/helper/Reply.cc index ae9f8faf1d..4624c67b19 100644 --- a/src/helper/Reply.cc +++ b/src/helper/Reply.cc @@ -16,29 +16,40 @@ #include "rfc1738.h" #include "SquidString.h" -Helper::Reply::Reply(char *buf, size_t len) : +Helper::Reply::Reply() : result(Helper::Unknown), whichServer(NULL) { - parse(buf,len); +} + +bool +Helper::Reply::accumulate(const char *buf, size_t len) +{ + if (other_.isNull()) + other_.init(4*1024, 1*1024*1024); + + if (other_.potentialSpaceSize() < static_cast(len)) + return false; // no space left + + other_.append(buf, len); + return true; } void -Helper::Reply::parse(char *buf, size_t len) +Helper::Reply::finalize() { debugs(84, 3, "Parsing helper buffer"); // check we have something to parse - if (!buf || len < 1) { + if (!other_.hasContent()) { // empty line response was the old URL-rewriter interface ERR response. result = Helper::Error; // for now ensure that legacy handlers are not presented with NULL strings. - debugs(84, 3, "Reply length is smaller than 1 or none at all "); - other_.init(1,1); - other_.terminate(); + debugs(84, 3, "Zero length reply"); return; } - char *p = buf; + char *p = other_.content(); + size_t len = other_.contentSize(); bool sawNA = false; // optimization: do not consider parsing result code if the response is short. @@ -67,10 +78,8 @@ Helper::Reply::parse(char *buf, size_t len) // followed by an auth token char *w1 = strwordtok(NULL, &p); if (w1 != NULL) { - MemBuf authToken; - authToken.init(); - authToken.append(w1, strlen(w1)); - notes.add("token",authToken.content()); + const char *authToken = w1; + notes.add("token",authToken); } else { // token field is mandatory on this response code result = Helper::BrokenHelper; @@ -88,22 +97,16 @@ Helper::Reply::parse(char *buf, size_t len) char *w2 = strwordtok(NULL, &p); if (w2 != NULL) { // Negotiate "token user" - MemBuf authToken; - authToken.init(); - authToken.append(w1, strlen(w1)); - notes.add("token",authToken.content()); + const char *authToken = w1; + notes.add("token",authToken); - MemBuf user; - user.init(); - user.append(w2,strlen(w2)); - notes.add("user",user.content()); + const char *user = w2; + notes.add("user",user); } else if (w1 != NULL) { // NTLM "user" - MemBuf user; - user.init(); - user.append(w1,strlen(w1)); - notes.add("user",user.content()); + const char *user = w1; + notes.add("user",user); } } else if (!strncmp(p,"NA ",3)) { // NTLM fail-closed ERR response @@ -115,21 +118,17 @@ Helper::Reply::parse(char *buf, size_t len) for (; xisspace(*p); ++p); // skip whitespace } - const mb_size_t blobSize = (buf+len-p); - other_.init(blobSize+1, blobSize+1); - other_.append(p, blobSize); // remainders of the line. - - // NULL-terminate so the helper callback handlers do not buffer-overrun - other_.terminate(); + other_.consume(p - other_.content()); + other_.consumeWhitespacePrefix(); // Hack for backward-compatibility: Do not parse for kv-pairs on NA response if (!sawNA) parseResponseKeys(); // Hack for backward-compatibility: BH and NA used to be a text message... - if (other().hasContent() && (sawNA || result == Helper::BrokenHelper)) { - notes.add("message",other().content()); - modifiableOther().clean(); + if (other_.hasContent() && (sawNA || result == Helper::BrokenHelper)) { + notes.add("message", other_.content()); + other_.clean(); } } @@ -157,8 +156,9 @@ void Helper::Reply::parseResponseKeys() { // parse a "key=value" pair off the 'other()' buffer. - while (other().hasContent()) { - char *p = modifiableOther().content(); + while (other_.hasContent()) { + char *p = other_.content(); + const char *key = p; while (*p && isKeyNameChar(*p)) ++p; if (*p != '=') return; // done. Not a key. @@ -171,8 +171,6 @@ Helper::Reply::parseResponseKeys() *p = '\0'; ++p; - const char *key = other().content(); - // the value may be a quoted string or a token const bool urlDecode = (*p != '"'); // check before moving p. char *v = strwordtok(NULL, &p); @@ -181,11 +179,20 @@ Helper::Reply::parseResponseKeys() notes.add(key, v ? v : ""); // value can be empty, but must not be NULL - modifiableOther().consume(p - other().content()); - modifiableOther().consumeWhitespacePrefix(); + other_.consume(p - other_.content()); + other_.consumeWhitespacePrefix(); } } +const MemBuf & +Helper::Reply::emptyBuf() const +{ + static MemBuf empty; + if (empty.isNull()) + empty.init(1, 1); + return empty; +} + std::ostream & operator <<(std::ostream &os, const Helper::Reply &r) { @@ -218,8 +225,9 @@ operator <<(std::ostream &os, const Helper::Reply &r) os << "}"; } - if (r.other().hasContent()) - os << ", other: \"" << r.other().content() << '\"'; + MemBuf const &o = r.other(); + if (o.hasContent()) + os << ", other: \"" << o.content() << '\"'; os << '}'; diff --git a/src/helper/Reply.h b/src/helper/Reply.h index 38e768754d..b0041367da 100644 --- a/src/helper/Reply.h +++ b/src/helper/Reply.h @@ -33,22 +33,12 @@ private: Reply &operator =(const Helper::Reply &r); public: - explicit Reply(Helper::ResultCode res = Helper::Unknown) : result(res), notes(), whichServer(NULL) { - other_.init(1,1); - other_.terminate(); - } + explicit Reply(Helper::ResultCode res) : result(res), notes(), whichServer(NULL) {} - // create/parse details from the msg buffer provided - // XXX: buf should be const but parse() needs non-const for now - Reply(char *buf, size_t len); + /// Creates a NULL reply + Reply(); - const MemBuf &other() const { return other_; } - - /// backward compatibility: - /// access to modifiable blob, required by redirectHandleReply() - /// and by urlParse() in ClientRequestContext::clientRedirectDone() - /// and by token blob/arg parsing in Negotiate auth handler - MemBuf &modifiableOther() const { return *const_cast(&other_); } + const MemBuf &other() const {return other_.isNull() ? emptyBuf() : other_;}; /** parse a helper response line format: * line := [ result ] *#( kv-pair ) @@ -58,7 +48,10 @@ public: * quoted-string are \-escape decoded and the quotes are stripped. */ // XXX: buf should be const but we may need strwordtok() and rfc1738_unescape() - void parse(char *buf, size_t len); + //void parse(char *buf, size_t len); + void finalize(); + + bool accumulate(const char *buf, size_t len); public: /// The helper response 'result' field. @@ -73,6 +66,9 @@ public: private: void parseResponseKeys(); + /// Return an empty MemBuf. + const MemBuf &emptyBuf() const; + /// the remainder of the line MemBuf other_; }; diff --git a/src/redirect.cc b/src/redirect.cc index c6bffaa543..945d58bff1 100644 --- a/src/redirect.cc +++ b/src/redirect.cc @@ -94,25 +94,28 @@ redirectHandleReply(void *data, const Helper::Reply &reply) // * trim all but the first word off the response. // * warn once every 50 responses that this will stop being fixed-up soon. // - if (const char * res = reply.other().content()) { + if (reply.other().hasContent()) { + const char * res = reply.other().content(); + size_t replySize = 0; if (const char *t = strchr(res, ' ')) { static int warn = 0; debugs(61, (!(warn++%50)? DBG_CRITICAL:2), "UPGRADE WARNING: URL rewriter reponded with garbage '" << t << "'. Future Squid will treat this as part of the URL."); - const mb_size_t garbageLength = reply.other().contentSize() - (t-res); - reply.modifiableOther().truncate(garbageLength); - } - if (reply.other().hasContent() && *res == '\0') - reply.modifiableOther().clean(); // drop the whole buffer of garbage. + replySize = t - res; + } else + replySize = reply.other().contentSize(); // if we still have anything in other() after all that // parse it into status=, url= and rewrite-url= keys - if (reply.other().hasContent()) { + if (replySize) { /* 2012-06-28: This cast is due to urlParse() truncating too-long URLs itself. * At this point altering the helper buffer in that way is not harmful, but annoying. * When Bug 1961 is resolved and urlParse has a const API, this needs to die. */ - char * result = reply.modifiableOther().content(); + MemBuf replyBuffer; + replyBuffer.init(replySize, replySize); + replyBuffer.append(reply.other().content(), reply.other().contentSize()); + char * result = replyBuffer.content(); Helper::Reply newReply; // BACKWARD COMPATIBILITY 2012-06-15: diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc index dfb8cb2d0c..7751d8db7c 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -85,8 +85,7 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void std::string msg = message.compose(); msg += '\n'; if (!ssl_crtd->trySubmit(msg.c_str(), callback, data)) { - ::Helper::Reply failReply; - failReply.result = ::Helper::BrokenHelper; + ::Helper::Reply failReply(::Helper::BrokenHelper); failReply.notes.add("message", "error 45 Temporary network problem, please retry later"); callback(data, failReply); return; @@ -198,6 +197,9 @@ sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply) if (reply.result == ::Helper::BrokenHelper) { debugs(83, DBG_IMPORTANT, "\"ssl_crtvd\" helper error response: " << reply.other().content()); validationResponse->resultCode = ::Helper::BrokenHelper; + } else if (!reply.other().hasContent()) { + debugs(83, DBG_IMPORTANT, "\"ssl_crtvd\" helper returned NULL response"); + validationResponse->resultCode = ::Helper::BrokenHelper; } else if (replyMsg.parse(reply.other().content(), reply.other().contentSize()) != Ssl::CrtdMessage::OK || !replyMsg.parseResponse(*validationResponse, peerCerts, error) ) { debugs(83, DBG_IMPORTANT, "WARNING: Reply from ssl_crtvd for " << " is incorrect");