Bug 3214: "helperHandleRead: unexpected read from ssl_crtd" errors.

Squid would read the beginning of a crtd response split across multiple
read operations and treat it as a complete response, causing various
certificate-related errors.

This patch:
 - allow the use of other than the '\n' character as the end of message mark
   for helper responses.
 - Use the '\1' char as end-of-message char for crtd helper. This char looks
   safe because the crtd messages are clear text only messages.

diff --git a/src/helper.cc b/src/helper.cc
index ea367d1c610b29fa8cdd9b735125df8176d9b8eb..b9ef32b772244bc6681326c8c38c4d2ea34ded4c 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -656,6 +656,7 @@ helperCreate(const char *name)
     CBDATA_INIT_TYPE(helper);
     hlp = cbdataAlloc(helper);
     hlp->id_name = name;
+    hlp->eom = '\n';
     return hlp;
 }
 
@@ -666,6 +667,7 @@ helperStatefulCreate(const char *name)
     CBDATA_INIT_TYPE(statefulhelper);
     hlp = cbdataAlloc(statefulhelper);
     hlp->id_name = name;
+    hlp->eom = '\n';
     return hlp;
 }
 
@@ -925,30 +927,25 @@ helperHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xerrno, voi
         srv->rbuf[0] = '\0';
     }
 
-    if (hlp->return_full_reply) {
-        debugs(84, 3, HERE << "Return entire buffer");
-        helperReturnBuffer(0, srv, hlp, srv->rbuf, srv->rbuf + srv->roffset);
-    } else {
-        while ((t = strchr(srv->rbuf, '\n'))) {
-            /* end of reply found */
-            char *msg = srv->rbuf;
-            int i = 0;
-            debugs(84, 3, "helperHandleRead: end of reply found");
-
-            if (t > srv->rbuf && t[-1] == '\r')
-                t[-1] = '\0';
+    while ((t = strchr(srv->rbuf, hlp->eom))) {
+        /* end of reply found */
+        char *msg = srv->rbuf;
+        int i = 0;
+        debugs(84, 3, "helperHandleRead: end of reply found");
 
-            *t++ = '\0';
+        if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n')
+            t[-1] = '\0';
 
-            if (hlp->concurrency) {
-                i = strtol(msg, &msg, 10);
+        *t++ = '\0';
 
-                while (*msg && xisspace(*msg))
-                    msg++;
-            }
+        if (hlp->concurrency) {
+            i = strtol(msg, &msg, 10);
 
-            helperReturnBuffer(i, srv, hlp, msg, t);
+            while (*msg && xisspace(*msg))
+                msg++;
         }
+
+        helperReturnBuffer(i, srv, hlp, msg, t);
     }
 
     if (srv->rfd != -1)
@@ -994,12 +991,12 @@ helperStatefulHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xer
         srv->roffset = 0;
     }
 
-    if ((t = strchr(srv->rbuf, '\n'))) {
+    if ((t = strchr(srv->rbuf, hlp->eom))) {
         /* end of reply found */
         int called = 1;
         debugs(84, 3, "helperStatefulHandleRead: end of reply found");
 
-        if (t > srv->rbuf && t[-1] == '\r')
+        if (t > srv->rbuf && t[-1] == '\r' && hlp->eom == '\n')
             t[-1] = '\0';
 
         *t = '\0';

diff --git a/src/helper.h b/src/helper.h
index ecef39d5ff79fdc088a74f29de439b049ac5b81c..317f024ea8d3db8bdc6b0695a91064f22112b47f 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -66,6 +66,7 @@ struct _helper {
     unsigned int concurrency;
     time_t last_queue_warn;
     time_t last_restart;
+    char eom;   ///< The char which marks the end of (response) message, normally '\n'
 
     struct {
         int requests;
@@ -73,8 +74,6 @@ struct _helper {
         int queue_size;
         int avg_svc_time;
     } stats;
-    /// True if callback expects the whole helper output, as a c-string.
-    bool return_full_reply;
 };
 
 struct _helper_stateful {
@@ -92,6 +91,7 @@ struct _helper_stateful {
     HLPSONEQ *OnEmptyQueue;
     time_t last_queue_warn;
     time_t last_restart;
+    char eom;   ///< The char which marks the end of (response) message, normally '\n'
 
     struct {
         int requests;

diff --git a/src/ssl/crtd_message.cc b/src/ssl/crtd_message.cc
index 9ae2808a841c1aba7d2a32d5c07e8bfcfcc827ee..de4f70df15d28b84cba0a0cb654610293837ca53 100644 (file)
--- a/src/ssl/crtd_message.cc
+++ b/src/ssl/crtd_message.cc
@@ -126,7 +126,7 @@ std::string Ssl::CrtdMessage::compose() const
     if (code.empty()) return std::string();
     char buffer[10];
     snprintf(buffer, sizeof(buffer), "%zd", body.length());
-    return code + ' ' + buffer + ' ' + body + '\n';
+    return code + ' ' + buffer + ' ' + body;
 }
 
 void Ssl::CrtdMessage::clear()

diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc
index 4d6f990a143767c5c8e48de735fc44462082ba15..438eb6c07fa1357d9f3d102f39a76a74dc9fa74a 100644 (file)
--- a/src/ssl/helper.cc
+++ b/src/ssl/helper.cc
@@ -30,6 +30,9 @@ void Ssl::Helper::Init()
         ssl_crtd = helperCreate("ssl_crtd");
     ssl_crtd->n_to_start = Ssl::TheConfig.ssl_crtd_n_running;
     ssl_crtd->ipc_type = IPC_STREAM;
+    // The crtd messages may contain the eol ('\n') character. We are
+    // going to use the '\1' char as the end-of-message mark.
+    ssl_crtd->eom = '\1';
     assert(ssl_crtd->cmdline == NULL);
     {
         char *tmp = xstrdup(Ssl::TheConfig.ssl_crtd);
@@ -57,7 +60,6 @@ void Ssl::Helper::Init()
         }
         safe_free(tmp_begin);
     }
-    ssl_crtd->return_full_reply = true;
     helperOpenServers(ssl_crtd);
 }
 
@@ -88,5 +90,7 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void
     }
 
     first_warn = 0;
-    helperSubmit(ssl_crtd, message.compose().c_str(), callback, data);
+    std::string msg = message.compose();
+    msg += '\n';
+    helperSubmit(ssl_crtd, msg.c_str(), callback, data);
 }

diff --git a/src/ssl/ssl_crtd.cc b/src/ssl/ssl_crtd.cc
index a357285cd0753af30bcc531505d0a668934a4845..b838fd481c1abb069a23900aa87f16ef37df408e 100644 (file)
--- a/src/ssl/ssl_crtd.cc
+++ b/src/ssl/ssl_crtd.cc
@@ -250,7 +250,8 @@ static bool proccessNewRequest(Ssl::CrtdMessage const & request_message, std::st
     response_message.setCode("ok");
     response_message.setBody(bufferToWrite);
 
-    std::cout << response_message.compose();
+    // Use the '\1' char as end-of-message character
+    std::cout << response_message.compose() << '\1' << std::flush;
 
     return true;
 }