Increase helper-to-Squid buffer size and warn on overflows. ssl_crtd helper may produce responses exceeding 9907 bytes in size (and possibly much larger if multiple chained certificates need to be returned to Squid). The old helper.cc code would fill the 8KB read buffer completely, schedule a read for zero bytes, receive zero bytes, declare an EOF condition, and close the stream (which kills ssl_crtd). Due to insufficient information logged, the observable symptoms were pretty much the same as if ssl_crtd closed the stream first, indicating an ssl_crtd bug. BUF_8KB comments indicated that other helpers may use larger-then-8KB messages as well although no specific cases were identified. We now warn if the read buffer reaches its capacity and kill the offending helper explicitly (because we cannot safely recover from this condition without implementing growing message accumulation buffers). And an increase in minimum buffer capacity to 32KB should make such events rare. TODO: This minimal patch does not attempt to fix many other problems with helper code, including rampant code duplication and fixed message buffer size. === modified file 'src/helper.cc' --- src/helper.cc 2012-01-20 18:55:04 +0000 +++ src/helper.cc 2012-02-27 17:54:59 +0000 @@ -30,44 +30,47 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA. * */ #include "squid-old.h" #include "base/AsyncCbdataCalls.h" #include "comm.h" #include "comm/Connection.h" #include "comm/Write.h" #include "helper.h" #include "format/Quoting.h" #include "MemBuf.h" #include "SquidMath.h" #include "SquidTime.h" #include "Store.h" #include "wordlist.h" #define HELPER_MAX_ARGS 64 -/* size of helper read buffer (maximum?). no reason given for this size */ -/* though it has been seen to be too short for some requests */ -/* it is dynamic, so increasng should not have side effects */ -#define BUF_8KB 8192 +/* 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. + * TODO: Replace a fixed-size buffer with a growing MemBuf. + */ +const size_t ReadBufMinSize(32*1024); // memAllocBuf() might allocate more 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 helper_request *Dequeue(helper * hlp); static helper_stateful_request *StatefulDequeue(statefulhelper * hlp); 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_stateful_request * r); static void helperKickQueue(helper * hlp); static void helperStatefulKickQueue(statefulhelper * hlp); static void helperStatefulServerDone(helper_stateful_server * srv); static void helperRequestFree(helper_request * r); static void helperStatefulRequestFree(helper_stateful_request * r); static void StatefulEnqueue(statefulhelper * hlp, helper_stateful_request * r); static bool helperStartStats(StoreEntry *sentry, void *hlp, const char *label); @@ -195,41 +198,41 @@ &wfd, &hIpc); if (pid < 0) { debugs(84, 1, "WARNING: Cannot run '" << progname << "' process."); continue; } hlp->childs.n_running++; hlp->childs.n_active++; CBDATA_INIT_TYPE(helper_server); srv = cbdataAlloc(helper_server); srv->hIpc = hIpc; srv->pid = pid; srv->index = k; srv->addr = hlp->addr; srv->readPipe = new Comm::Connection; srv->readPipe->fd = rfd; srv->writePipe = new Comm::Connection; srv->writePipe->fd = wfd; - srv->rbuf = (char *)memAllocBuf(BUF_8KB, &srv->rbuf_sz); + srv->rbuf = (char *)memAllocBuf(ReadBufMinSize, &srv->rbuf_sz); srv->wqueue = new MemBuf; srv->roffset = 0; srv->requests = (helper_request **)xcalloc(hlp->childs.concurrency ? hlp->childs.concurrency : 1, sizeof(*srv->requests)); srv->parent = cbdataReference(hlp); dlinkAddTail(srv, &srv->link, &hlp->servers); if (rfd == wfd) { snprintf(fd_note_buf, FD_DESC_SZ, "%s #%d", shortname, k + 1); fd_note(rfd, fd_note_buf); } else { snprintf(fd_note_buf, FD_DESC_SZ, "reading %s #%d", shortname, k + 1); fd_note(rfd, fd_note_buf); snprintf(fd_note_buf, FD_DESC_SZ, "writing %s #%d", shortname, k + 1); fd_note(wfd, fd_note_buf); } commSetNonBlocking(rfd); if (wfd != rfd) commSetNonBlocking(wfd); @@ -314,41 +317,41 @@ if (pid < 0) { debugs(84, 1, "WARNING: Cannot run '" << progname << "' process."); continue; } hlp->childs.n_running++; hlp->childs.n_active++; CBDATA_INIT_TYPE(helper_stateful_server); helper_stateful_server *srv = cbdataAlloc(helper_stateful_server); srv->hIpc = hIpc; srv->pid = pid; srv->flags.reserved = 0; srv->stats.submits = 0; srv->stats.releases = 0; srv->index = k; srv->addr = hlp->addr; srv->readPipe = new Comm::Connection; srv->readPipe->fd = rfd; srv->writePipe = new Comm::Connection; srv->writePipe->fd = wfd; - srv->rbuf = (char *)memAllocBuf(BUF_8KB, &srv->rbuf_sz); + srv->rbuf = (char *)memAllocBuf(ReadBufMinSize, &srv->rbuf_sz); srv->roffset = 0; srv->parent = cbdataReference(hlp); if (hlp->datapool != NULL) srv->data = hlp->datapool->alloc(); dlinkAddTail(srv, &srv->link, &hlp->servers); if (rfd == wfd) { snprintf(fd_note_buf, FD_DESC_SZ, "%s #%d", shortname, k + 1); fd_note(rfd, fd_note_buf); } else { snprintf(fd_note_buf, FD_DESC_SZ, "reading %s #%d", shortname, k + 1); fd_note(rfd, fd_note_buf); snprintf(fd_note_buf, FD_DESC_SZ, "writing %s #%d", shortname, k + 1); fd_note(wfd, fd_note_buf); } commSetNonBlocking(rfd); @@ -887,43 +890,56 @@ char *msg = srv->rbuf; int i = 0; debugs(84, 3, "helperHandleRead: end of reply found"); if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n') t[-1] = '\0'; *t++ = '\0'; if (hlp->childs.concurrency) { i = strtol(msg, &msg, 10); while (*msg && xisspace(*msg)) msg++; } helperReturnBuffer(i, srv, hlp, msg, t); } if (Comm::IsConnOpen(srv->readPipe)) { + const int 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 + 1)); + srv->closePipesSafely(); + return; + } + AsyncCall::Pointer call = commCbCall(5,4, "helperHandleRead", CommIoCbPtrFun(helperHandleRead, srv)); - comm_read(srv->readPipe, srv->rbuf + srv->roffset, srv->rbuf_sz - srv->roffset - 1, call); + comm_read(srv->readPipe, srv->rbuf + srv->roffset, spaceSize, call); } } static void helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm_err_t flag, int xerrno, void *data) { char *t = NULL; helper_stateful_server *srv = (helper_stateful_server *)data; helper_stateful_request *r; statefulhelper *hlp = srv->parent; assert(cbdataReferenceValid(data)); /* Bail out early on COMM_ERR_CLOSING - close handlers will tidy up for us */ if (flag == COMM_ERR_CLOSING) { return; } assert(conn->fd == srv->readPipe->fd); @@ -967,43 +983,56 @@ } srv->flags.busy = 0; srv->roffset = 0; helperStatefulRequestFree(r); srv->request = NULL; hlp->stats.replies++; srv->answer_time = current_time; hlp->stats.avg_svc_time = Math::intAverage(hlp->stats.avg_svc_time, tvSubMsec(srv->dispatch_time, current_time), hlp->stats.replies, REDIRECT_AV_FACTOR); if (called) helperStatefulServerDone(srv); else helperStatefulReleaseServer(srv); } if (Comm::IsConnOpen(srv->readPipe)) { + const int 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 + 1)); + srv->closePipesSafely(); + return; + } + AsyncCall::Pointer call = commCbCall(5,4, "helperStatefulHandleRead", CommIoCbPtrFun(helperStatefulHandleRead, srv)); - comm_read(srv->readPipe, srv->rbuf + srv->roffset, srv->rbuf_sz - srv->roffset - 1, call); + comm_read(srv->readPipe, srv->rbuf + srv->roffset, spaceSize, call); } } static void Enqueue(helper * hlp, helper_request * r) { dlink_node *link = (dlink_node *)memAllocate(MEM_DLINK_NODE); dlinkAddTail(r, link, &hlp->queue); hlp->stats.queue_size++; /* do this first so idle=N has a chance to grow the child pool before it hits critical. */ if (hlp->childs.needNew() > 0) { debugs(84, 0, "Starting new " << hlp->id_name << " helpers..."); helperOpenServers(hlp); return; } if (hlp->stats.queue_size < (int)hlp->childs.n_running) return;