From 51e23b582e907c31f22bd157dadb81fa2f684b43 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Fri, 17 Jun 2011 06:36:07 -0600 Subject: [PATCH] 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. --- src/helper.cc | 39 ++++++++++++++++++--------------------- src/helper.h | 4 ++-- src/ssl/crtd_message.cc | 2 +- src/ssl/helper.cc | 8 ++++++-- src/ssl/ssl_crtd.cc | 3 ++- 5 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/helper.cc b/src/helper.cc index ea367d1c61..b9ef32b772 100644 --- 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 ecef39d5ff..317f024ea8 100644 --- 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 9ae2808a84..de4f70df15 100644 --- 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 4d6f990a14..438eb6c07f 100644 --- 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 a357285cd0..b838fd481c 100644 --- 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; } -- 2.47.2