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.

diff --git a/src/helper.cc b/src/helper.cc
index d6ded5aff31dbb0607ede05f512296765ac73b88..000031d6c2e5b9b50b3ba011f816955ce02c0500 100644 (file)
--- 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);