From dd8b7fd72a9c65e497aa1b75c910067c97c9f273 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Sat, 23 Sep 2023 18:51:05 +0000 Subject: [PATCH] Kill helpers that speak without being spoken to (#1488) ERROR: helperHandleRead: unexpected read from ... ERROR: helperStatefulHandleRead: unexpected read from ... Squid ignored bytes received from both stateful and stateless helper processes that had no outstanding helper requests at the time of read(2). In stateful helpers, the implementation also resulted in undefined behavior: Calling std::list::front() with an empty list. Ignoring these "early" bytes also complicates code improvements. Detecting early bytes cannot be done reliably because Squid cannot know whether some early bytes were sent just before Squid created a helper request and, hence, could be mistaken for a helper response to that request. Incorrectly mapping helper responses could lead to serious problems. When Squid is lucky to detect a buggy helper that sends early bytes, the safest and simplest action is to kill the helper process. --- src/helper.cc | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/helper.cc b/src/helper.cc index d6ded5aff3..000031d6c2 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -1040,12 +1040,14 @@ helperHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len, Comm:: if (!srv->stats.pending && !srv->stats.timedout) { /* someone spoke without being spoken to */ - debugs(84, DBG_IMPORTANT, "ERROR: helperHandleRead: unexpected read from " << + debugs(84, DBG_IMPORTANT, "ERROR: Killing helper process after an unexpected read from " << hlp->id_name << " #" << srv->index << ", " << (int)len << " bytes '" << srv->rbuf << "'"); srv->roffset = 0; srv->rbuf[0] = '\0'; + srv->closePipesSafely(hlp->id_name); + return; } bool needsMore = false; @@ -1154,16 +1156,17 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len srv->roffset += len; srv->rbuf[srv->roffset] = '\0'; - Helper::Xaction *r = srv->requests.front(); debugs(84, DBG_DATA, Raw("accumulated", srv->rbuf, srv->roffset)); - if (r == nullptr) { + if (srv->requests.empty()) { /* someone spoke without being spoken to */ - debugs(84, DBG_IMPORTANT, "ERROR: helperStatefulHandleRead: unexpected read from " << + debugs(84, DBG_IMPORTANT, "ERROR: Killing helper process after an unexpected read from " << hlp->id_name << " #" << srv->index << ", " << (int)len << " bytes '" << srv->rbuf << "'"); srv->roffset = 0; + srv->closePipesSafely(hlp->id_name); + return; } if ((t = strchr(srv->rbuf, hlp->eom))) { @@ -1178,7 +1181,9 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len *t = '\0'; } - if (r && !r->reply.accumulate(srv->rbuf, t ? (t - srv->rbuf) : srv->roffset)) { + const auto r = srv->requests.front(); + + 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); @@ -1198,7 +1203,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len srv->requests.pop_front(); // we already have it in 'r' int called = 1; - if (r && cbdataReferenceValid(r->request.data)) { + if (cbdataReferenceValid(r->request.data)) { r->reply.finalize(); r->reply.reservationId = srv->reservationId; r->request.callback(r->request.data, r->reply); -- 2.47.2