Support OK/ERR/BH response codes from any helper

Updates the helper reponse callback API from using char* buffer to a
HelperReply object storing teh response code, a blob buffer, and
pointer to the responding helper 'server' (if stateful).

* the helper I/O read handler is updated to parse the result code off
the start of the helper response as is currently done for channel-ID.
The callback handlers are altered to use the HelperReply::status instead
of parsing it off themselves individually.

* the remaining I/O read buffer is stored in a MemBuf and callbacks are
updated to use it via the method other().

* the responding helper-server is stored into the HelperReply object and
stateful helper callbacks are combined into the same API as stateless.
The callback handlers are updated to use HelperReply::lastserver instead
of function parameter.

After this patch the helper response format is:
  [channel-ID] SP [result] [ [SP] blob] <terminator>

'SP' being one octet \0x20 character.

The behavour changes expected from this is that all helpers are now able
to send OK/ERR/BH states. Although the handlers for some helpers will
deal with the new states as unknown response. None of the bundled
helpers have yet been altered to make use of this changed potential.

TODO:
* implement key=value parser for the blob area of the format, and update
handlers to use the HelperReply API to retrieve them.
* upgrade helpers to make use of new response format

diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h
index 3c7f817c7e8e799408b3bcae313bc7f46019125d..28108d9300af673e870478a33dc9949c18b0bd24 100644 (file)
--- a/src/ClientRequestContext.h
+++ b/src/ClientRequestContext.h
@@ -14,6 +14,8 @@ class ClientHttpRequest;
 class DnsLookupDetails;
 class ErrorState;
 
+class HelperReply;
+
 class ClientRequestContext : public RefCountable
 {
 
@@ -32,7 +34,7 @@ public:
     void clientAccessCheck2();
     void clientAccessCheckDone(const allow_t &answer);
     void clientRedirectStart();
-    void clientRedirectDone(char *result);
+    void clientRedirectDone(const HelperReply &reply);
     void checkNoCache();
     void checkNoCacheDone(const allow_t &answer);
 #if USE_ADAPTATION

diff --git a/src/HelperReply.cc b/src/HelperReply.cc
new file mode 100644 (file)
index 0000000..983dabd
--- /dev/null
+++ b/src/HelperReply.cc
@@ -0,0 +1,94 @@
+#include "squid.h"
+#include "HelperReply.h"
+#include "helper.h"
+
+HelperReply::HelperReply(const char *buf, size_t len) :
+        result(HelperReply::Unknown),
+        whichServer(NULL)
+{
+    // check we have something to parse
+    if (!buf || len < 1) {
+        // for now ensure that legacy handlers are not presented with NULL strings.
+        other_.init(1,1);
+        other_.terminate();
+        return;
+    }
+
+    const char *p = buf;
+
+    // optimization: do not consider parsing result code if the response is short.
+    // URL-rewriter may return relative URLs or empty response for a large portion
+    // of its replies.
+    if (len >= 2) {
+        // some helper formats (digest auth, URL-rewriter) just send a data string
+        // we must also check for the ' ' character after the response token (if anything)
+        if (!strncmp(p,"OK",2) && (len == 2 || p[2] == ' ')) {
+            result = HelperReply::Okay;
+            p+=2;
+        } else if (!strncmp(p,"ERR",3) && (len == 3 || p[3] == ' ')) {
+            result = HelperReply::Error;
+            p+=3;
+        } else if (!strncmp(p,"BH",2) && (len == 2 || p[2] == ' ')) {
+            result = HelperReply::BrokenHelper;
+            p+=2;
+        } else if (!strncmp(p,"TT ",3)) {
+            // NTLM challenge token
+            result = HelperReply::TT;
+            p+=3;
+        } else if (!strncmp(p,"AF ",3)) {
+            // NTLM OK response
+            result = HelperReply::AF;
+            p+=3;
+        } else if (!strncmp(p,"NA ",3)) {
+            // NTLM fail-closed ERR response
+            result = HelperReply::NA;
+            p+=3;
+        }
+
+        for(;xisspace(*p);++p); // skip whitespace
+    }
+
+    const mb_size_t blobSize = (buf+len-p);
+    other_.init(blobSize+1, blobSize+1);
+    other_.append(p, blobSize); // remainders of the line.
+
+    // NULL-terminate so the helper callback handlers do not buffer-overrun
+    other_.terminate();
+}
+
+std::ostream &
+operator <<(std::ostream &os, const HelperReply &r)
+{
+    os << "{result=";
+    switch(r.result)
+    {
+    case HelperReply::Okay:
+        os << "OK";
+        break;
+    case HelperReply::Error:
+        os << "ERR";
+        break;
+    case HelperReply::BrokenHelper:
+        os << "BH";
+        break;
+    case HelperReply::TT:
+        os << "TT";
+        break;
+    case HelperReply::AF:
+        os << "AF";
+        break;
+    case HelperReply::NA:
+        os << "NA";
+        break;
+    case HelperReply::Unknown:
+        os << "Unknown";
+        break;
+    }
+
+    if (r.other().hasContent())
+        os << ", other: \"" << r.other().content() << '\"';
+
+    os << '}';
+
+    return os;
+}

diff --git a/src/HelperReply.h b/src/HelperReply.h
new file mode 100644 (file)
index 0000000..ea1657d
--- /dev/null
+++ b/src/HelperReply.h
@@ -0,0 +1,65 @@
+#ifndef _SQUID_SRC_HELPERREPLY_H
+#define _SQUID_SRC_HELPERREPLY_H
+
+#include "base/CbcPointer.h"
+#include "MemBuf.h"
+
+#if HAVE_OSTREAM
+#include <ostream>
+#endif
+
+class helper_stateful_server;
+
+/**
+ * This object stores the reply message from a helper lookup
+ * It provides parser routing to accept a raw buffer and process the
+ * helper reply into fields for easy access by callers
+ */
+class HelperReply
+{
+private:
+    // copy are prohibited for now
+    HelperReply(const HelperReply &r);
+    HelperReply &operator =(const HelperReply &r);
+
+public:
+    // create/parse details from the msg buffer provided
+    HelperReply(const char *buf, size_t len);
+
+    const MemBuf &other() const { return other_; }
+
+    /// backward compatibility:
+    /// access to modifiable blob, required by redirectHandleReply()
+    /// and by urlParse() in ClientRequestContext::clientRedirectDone()
+    /// and by token blob/arg parsing in Negotiate auth handler
+    MemBuf &modifiableOther() const { return *const_cast<MemBuf*>(&other_); }
+
+public:
+    /// The helper response 'result' field.
+    enum Result_ {
+        Unknown,      // no result code received, or unknown result code
+        Okay,         // "OK" indicating success/positive result
+        Error,        // "ERR" indicating failure/negative result
+        BrokenHelper, // "BH" indicating failure due to helper internal problems.
+
+        // some result codes for backward compatibility with NTLM/Negotiate
+        // TODO: migrate these into variants of the above results with key-pair parameters
+        TT,
+        AF,
+        NA
+    } result;
+
+// TODO other key=pair values. when the callbacks actually use this object.
+// for now they retain their own parsing routines handling other()
+
+    /// for stateful replies the responding helper 'server' needs to be preserved across callbacks
+    CbcPointer<helper_stateful_server> whichServer;
+
+private:
+    /// the remainder of the line
+    MemBuf other_;
+};
+
+std::ostream &operator <<(std::ostream &os, const HelperReply &r);
+
+#endif /* _SQUID_SRC_HELPERREPLY_H */

diff --git a/src/Makefile.am b/src/Makefile.am
index f908ed2394ee1d9e4873f5560448354d3af86c1d..6da7b724e9fca4895d7192d369b95200bace2478 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -364,6 +364,8 @@ squid_SOURCES = \
        helper.h \
        HelperChildConfig.h \
        HelperChildConfig.cc \
+       HelperReply.cc \
+       HelperReply.h \
        hier_code.h \
        HierarchyLogEntry.h \
        $(HTCPSOURCE) \
@@ -1452,6 +1454,8 @@ tests_testCacheManager_SOURCES = \
        helper.cc \
        HelperChildConfig.h \
        HelperChildConfig.cc \
+       HelperReply.cc \
+       HelperReply.h \
        $(HTCPSOURCE) \
        HttpStateFlags.h \
        http.cc \
@@ -1858,6 +1862,8 @@ tests_testEvent_SOURCES = \
        helper.cc \
        HelperChildConfig.h \
        HelperChildConfig.cc \
+       HelperReply.cc \
+       HelperReply.h \
        hier_code.h \
        $(HTCPSOURCE) \
        http.cc \
@@ -2103,6 +2109,8 @@ tests_testEventLoop_SOURCES = \
        helper.cc \
        HelperChildConfig.h \
        HelperChildConfig.cc \
+       HelperReply.cc \
+       HelperReply.h \
        hier_code.h \
        $(HTCPSOURCE) \
        http.cc \
@@ -2345,6 +2353,8 @@ tests_test_http_range_SOURCES = \
        helper.cc \
        HelperChildConfig.h \
        HelperChildConfig.cc \
+       HelperReply.cc \
+       HelperReply.h \
        hier_code.h \
        $(HTCPSOURCE) \
        http.cc \
@@ -2637,6 +2647,8 @@ tests_testHttpRequest_SOURCES = \
        helper.cc \
        HelperChildConfig.h \
        HelperChildConfig.cc \
+       HelperReply.cc \
+       HelperReply.h \
        hier_code.h \
        $(HTCPSOURCE) \
        http.cc \
@@ -3724,6 +3736,8 @@ tests_testURL_SOURCES = \
        helper.cc \
        HelperChildConfig.h \
        HelperChildConfig.cc \
+       HelperReply.cc \
+       HelperReply.h \
        hier_code.h \
        $(HTCPSOURCE) \
        http.cc \

diff --git a/src/SquidDns.h b/src/SquidDns.h
index 83cc516cf6a0822f8e58e01f7c699c842b87eeab..ed44c0b4256b89e5058becf8bb70c75c1a0a4570 100644 (file)
--- a/src/SquidDns.h
+++ b/src/SquidDns.h
@@ -1,6 +1,10 @@
 #ifndef SQUID_DNS_H
 #define SQUID_DNS_H
 
+#if USE_DNSHELPER
+#include "helper.h"
+#endif
+
 namespace Ip
 {
 class Address;

diff --git a/src/auth/State.h b/src/auth/State.h
index 5722f2ad09381a84ff39b07d6531b438157777da..16970418e747b718d40ceb87f867b2e4a443435a 100644 (file)
--- a/src/auth/State.h
+++ b/src/auth/State.h
@@ -5,6 +5,7 @@
 
 #include "auth/UserRequest.h"
 #include "cbdata.h"
+#include "helper.h"
 
 namespace Auth
 {

diff --git a/src/auth/UserRequest.h b/src/auth/UserRequest.h
index 787bc29f820ac2adc6acb02de665a714e3f31314..f175573bbff3e1a3e20ee6c62168dd9df0a7dd18 100644 (file)
--- a/src/auth/UserRequest.h
+++ b/src/auth/UserRequest.h
@@ -37,7 +37,7 @@
 #include "auth/User.h"
 #include "dlink.h"
 #include "ip/Address.h"
-#include "typedefs.h"
+#include "helper.h"
 #include "HttpHeader.h"
 
 class ConnStateData;

diff --git a/src/auth/basic/UserRequest.cc b/src/auth/basic/UserRequest.cc
index a990818461e8581849c4a4ff5c0fec72426d36e9..a3d9651579b4e0d0912f2b7c01fbb225bed3871d 100644 (file)
--- a/src/auth/basic/UserRequest.cc
+++ b/src/auth/basic/UserRequest.cc
@@ -5,6 +5,7 @@
 #include "auth/State.h"
 #include "charset.h"
 #include "Debug.h"
+#include "HelperReply.h"
 #include "rfc1738.h"
 #include "SquidTime.h"
 
@@ -135,23 +136,12 @@ Auth::Basic::UserRequest::module_start(AUTHCB * handler, void *data)
 }
 
 void
-Auth::Basic::UserRequest::HandleReply(void *data, char *reply)
+Auth::Basic::UserRequest::HandleReply(void *data, const HelperReply &reply)
 {
     Auth::StateData *r = static_cast<Auth::StateData *>(data);
     BasicAuthQueueNode *tmpnode;
-    char *t = NULL;
     void *cbdata;
-    debugs(29, 5, HERE << "{" << (reply ? reply : "<NULL>") << "}");
-
-    if (reply) {
-        if ((t = strchr(reply, ' '))) {
-            *t = '\0';
-            ++t;
-        }
-
-        if (*reply == '\0')
-            reply = NULL;
-    }
+    debugs(29, 5, HERE << "reply=" << reply);
 
     assert(r->auth_user_request != NULL);
     assert(r->auth_user_request->user()->auth_type == Auth::AUTH_BASIC);
@@ -162,13 +152,13 @@ Auth::Basic::UserRequest::HandleReply(void *data, char *reply)
 
     assert(basic_auth != NULL);
 
-    if (reply && (strncasecmp(reply, "OK", 2) == 0))
+    if (reply.result == HelperReply::Okay)
         basic_auth->credentials(Auth::Ok);
     else {
         basic_auth->credentials(Auth::Failed);
 
-        if (t && *t)
-            r->auth_user_request->setDenyMessage(t);
+        if (reply.other().hasContent())
+            r->auth_user_request->setDenyMessage(reply.other().content());
     }
 
     basic_auth->expiretime = squid_curtime;

diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc
index 11764a319eb5b6a15ccf5ef0a06d013fae6d509d..2caea95f1b97dd42a238c99381bea6ac4a0bfdc0 100644 (file)
--- a/src/auth/digest/UserRequest.cc
+++ b/src/auth/digest/UserRequest.cc
@@ -272,27 +272,18 @@ Auth::Digest::UserRequest::module_start(AUTHCB * handler, void *data)
 }
 
 void
-Auth::Digest::UserRequest::HandleReply(void *data, char *reply)
+Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply)
 {
     Auth::StateData *replyData = static_cast<Auth::StateData *>(data);
-    char *t = NULL;
-    void *cbdata;
-    debugs(29, 9, HERE << "{" << (reply ? reply : "<NULL>") << "}");
-
-    if (reply) {
-        if ((t = strchr(reply, ' '))) {
-            *t = '\0';
-            ++t;
-        }
-
-        if (*reply == '\0' || *reply == '\n')
-            reply = NULL;
-    }
+    debugs(29, 9, HERE << "reply=" << reply);
 
     assert(replyData->auth_user_request != NULL);
     Auth::UserRequest::Pointer auth_user_request = replyData->auth_user_request;
 
-    if (reply && (strncasecmp(reply, "ERR", 3) == 0)) {
+    switch(reply.result)
+    {
+    case HelperReply::Error:
+    {
         /* allow this because the digest_request pointer is purely local */
         Auth::Digest::UserRequest *digest_request = dynamic_cast<Auth::Digest::UserRequest *>(auth_user_request.getRaw());
         assert(digest_request);
@@ -300,17 +291,28 @@ Auth::Digest::UserRequest::HandleReply(void *data, char *reply)
         digest_request->user()->credentials(Auth::Failed);
         digest_request->flags.invalid_password = 1;
 
-        if (t && *t)
-            digest_request->setDenyMessage(t);
-    } else if (reply) {
+        if (reply.other().hasContent())
+            digest_request->setDenyMessage(reply.other().content());
+    }
+        break;
+
+    case HelperReply::Unknown: // Squid 3.2 and older the digest helper only returns a HA1 hash (no "OK")
+    case HelperReply::Okay:
+    {
         /* allow this because the digest_request pointer is purely local */
         Auth::Digest::User *digest_user = dynamic_cast<Auth::Digest::User *>(auth_user_request->user().getRaw());
         assert(digest_user != NULL);
 
-        CvtBin(reply, digest_user->HA1);
+        CvtBin(reply.other().content(), digest_user->HA1);
         digest_user->HA1created = 1;
     }
+        break;
+
+    default:
+        ; // XXX: handle other states properly.
+    }
 
+    void *cbdata = NULL;
     if (cbdataReferenceValidDone(replyData->data, &cbdata))
         replyData->handler(cbdata);
 

diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc
index 1b0aef6299007d195cdb022df740744c5bd642df..19e99fce7806273e9379ce0f30fa3981630c8247 100644 (file)
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -232,25 +232,18 @@ Auth::Negotiate::UserRequest::authenticate(HttpRequest * aRequest, ConnStateData
 }
 
 void
-Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *reply)
+Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
 {
     Auth::StateData *r = static_cast<Auth::StateData *>(data);
 
-    char *blob, *arg = NULL;
-
-    debugs(29, 8, HERE << "helper: '" << lastserver << "' sent us '" << (reply ? reply : "<NULL>") << "'");
+    debugs(29, 8, HERE << "helper: '" << reply.whichServer << "' sent us reply=" << reply);
 
     if (!cbdataReferenceValid(r->data)) {
-        debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication invalid callback data. helper '" << lastserver << "'.");
+        debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication invalid callback data. helper '" << reply.whichServer << "'.");
         delete r;
         return;
     }
 
-    if (!reply) {
-        debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication Helper '" << lastserver << "' crashed!.");
-        reply = (char *)"BH Internal error";
-    }
-
     Auth::UserRequest::Pointer auth_user_request = r->auth_user_request;
     assert(auth_user_request != NULL);
 
@@ -265,26 +258,26 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *re
     assert(auth_user_request->user()->auth_type == Auth::AUTH_NEGOTIATE);
 
     if (lm_request->authserver == NULL)
-        lm_request->authserver = static_cast<helper_stateful_server*>(lastserver);
+        lm_request->authserver = reply.whichServer.get(); // XXX: no locking?
     else
-        assert(lm_request->authserver == lastserver);
+        assert(reply.whichServer == lm_request->authserver);
 
     /* seperate out the useful data */
-    blob = strchr(reply, ' ');
-
-    if (blob) {
-        ++blob;
-        arg = strchr(blob + 1, ' ');
-    } else {
-        arg = NULL;
-    }
-
-    if (strncasecmp(reply, "TT ", 3) == 0) {
-        /* we have been given a blob to send to the client */
+    char *modifiableBlob = reply.modifiableOther().content();
+    char *arg = NULL;
+    if (modifiableBlob && *modifiableBlob != '\0') {
+        arg = strchr(modifiableBlob + 1, ' ');
         if (arg) {
             *arg = '\0';
             ++arg;
         }
+    }
+    const char *blob = modifiableBlob;
+
+    switch(reply.result)
+    {
+    case HelperReply::TT:
+        /* we have been given a blob to send to the client */
         safe_free(lm_request->server_blob);
         lm_request->request->flags.mustKeepalive = 1;
         if (lm_request->request->flags.proxyKeepalive) {
@@ -296,14 +289,19 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *re
             auth_user_request->user()->credentials(Auth::Failed);
             auth_user_request->denyMessage("NTLM authentication requires a persistent connection");
         }
-    } else if (strncasecmp(reply, "AF ", 3) == 0 && arg != NULL) {
-        /* we're finished, release the helper */
+        break;
 
-        if (arg) {
-            *arg = '\0';
-            ++arg;
+    case HelperReply::AF:
+    case HelperReply::Okay:
+    {
+        if (arg == NULL) {
+            // XXX: handle a success with no username better
+            /* protocol error */
+            fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply.other().content());
+            break;
         }
 
+        /* we're finished, release the helper */
         auth_user_request->user()->username(arg);
         auth_user_request->denyMessage("Login successful");
         safe_free(lm_request->server_blob);
@@ -336,23 +334,32 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *re
          * existing user or a new user */
         local_auth_user->expiretime = current_time.tv_sec;
         auth_user_request->user()->credentials(Auth::Ok);
-        debugs(29, 4, HERE << "Successfully validated user via Negotiate. Username '" << blob << "'");
-
-    } else if (strncasecmp(reply, "NA ", 3) == 0 && arg != NULL) {
-        /* authentication failure (wrong password, etc.) */
+        debugs(29, 4, HERE << "Successfully validated user via Negotiate. Username '" << arg << "'");
+    }
+        break;
 
-        if (arg) {
-            *arg = '\0';
-            ++arg;
+    case HelperReply::NA:
+    case HelperReply::Error:
+        if (arg == NULL) {
+            /* protocol error */
+            fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply.other().content());
+            break;
         }
-
+        /* authentication failure (wrong password, etc.) */
         auth_user_request->denyMessage(arg);
         auth_user_request->user()->credentials(Auth::Failed);
         safe_free(lm_request->server_blob);
         lm_request->server_blob = xstrdup(blob);
         lm_request->releaseAuthServer();
-        debugs(29, 4, HERE << "Failed validating user via Negotiate. Error returned '" << blob << "'");
-    } else if (strncasecmp(reply, "BH ", 3) == 0) {
+        debugs(29, 4, HERE << "Failed validating user via Negotiate. Error returned '" << reply << "'");
+        break;
+
+    case HelperReply::Unknown:
+        debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication Helper '" << reply.whichServer << "' crashed!.");
+        blob = "Internal error";
+        /* continue to the next case */
+
+    case HelperReply::BrokenHelper:
         /* TODO kick off a refresh process. This can occur after a YR or after
          * a KK. If after a YR release the helper and resubmit the request via
          * Authenticate Negotiate start.
@@ -362,12 +369,10 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *re
         auth_user_request->user()->credentials(Auth::Failed);
         safe_free(lm_request->server_blob);
         lm_request->releaseAuthServer();
-        debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication validating user. Error returned '" << reply << "'");
-    } else {
-        /* protocol error */
-        fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply);
+        debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication validating user. Error returned " << reply);
     }
 
+    xfree(arg);
     if (lm_request->request) {
         HTTPMSGUNLOCK(lm_request->request);
         lm_request->request = NULL;

diff --git a/src/auth/negotiate/UserRequest.h b/src/auth/negotiate/UserRequest.h
index 478680cfcdee11428523964164306ee65c251fa3..79c0e19f19ff8251e75f1babf4160d81321692b6 100644 (file)
--- a/src/auth/negotiate/UserRequest.h
+++ b/src/auth/negotiate/UserRequest.h
@@ -52,7 +52,7 @@ public:
     HttpRequest *request;
 
 private:
-    static HLPSCB HandleReply;
+    static HLPCB HandleReply;
 };
 
 } // namespace Negotiate

diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 2d31ff4f3dddb23b3ede2d24ee5feebb1b1cb655..80f33f2e090135bedf23dee53aab32193faac4cc 100644 (file)
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -225,24 +225,18 @@ Auth::Ntlm::UserRequest::authenticate(HttpRequest * aRequest, ConnStateData * co
 }
 
 void
-Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply)
+Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply)
 {
     Auth::StateData *r = static_cast<Auth::StateData *>(data);
-    char *blob;
 
-    debugs(29, 8, HERE << "helper: '" << lastserver << "' sent us '" << (reply ? reply : "<NULL>") << "'");
+    debugs(29, 8, HERE << "helper: '" << reply.whichServer << "' sent us reply=" << reply);
 
     if (!cbdataReferenceValid(r->data)) {
-        debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication invalid callback data. helper '" << lastserver << "'.");
+        debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication invalid callback data. helper '" << reply.whichServer << "'.");
         delete r;
         return;
     }
 
-    if (!reply) {
-        debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication Helper '" << lastserver << "' crashed!.");
-        reply = (char *)"BH Internal error";
-    }
-
     Auth::UserRequest::Pointer auth_user_request = r->auth_user_request;
     assert(auth_user_request != NULL);
 
@@ -257,16 +251,16 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply)
     assert(auth_user_request->user()->auth_type == Auth::AUTH_NTLM);
 
     if (lm_request->authserver == NULL)
-        lm_request->authserver = static_cast<helper_stateful_server*>(lastserver);
+        lm_request->authserver = reply.whichServer.get(); // XXX: no locking?
     else
-        assert(lm_request->authserver == lastserver);
+        assert(reply.whichServer == lm_request->authserver);
 
     /* seperate out the useful data */
-    blob = strchr(reply, ' ');
-    if (blob)
-        ++blob;
+    const char *blob = reply.other().content();
 
-    if (strncasecmp(reply, "TT ", 3) == 0) {
+    switch(reply.result)
+    {
+    case HelperReply::TT:
         /* we have been given a blob to send to the client */
         safe_free(lm_request->server_blob);
         lm_request->request->flags.mustKeepalive = 1;
@@ -279,7 +273,11 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply)
             auth_user_request->user()->credentials(Auth::Failed);
             auth_user_request->denyMessage("NTLM authentication requires a persistent connection");
         }
-    } else if (strncasecmp(reply, "AF ", 3) == 0) {
+        break;
+
+    case HelperReply::AF:
+    case HelperReply::Okay:
+    {
         /* we're finished, release the helper */
         auth_user_request->user()->username(blob);
         auth_user_request->denyMessage("Login successful");
@@ -314,15 +312,25 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply)
         local_auth_user->expiretime = current_time.tv_sec;
         auth_user_request->user()->credentials(Auth::Ok);
         debugs(29, 4, HERE << "Successfully validated user via NTLM. Username '" << blob << "'");
+    }
+        break;
 
-    } else if (strncasecmp(reply, "NA ", 3) == 0) {
+    case HelperReply::NA:
+    case HelperReply::Error:
         /* authentication failure (wrong password, etc.) */
         auth_user_request->denyMessage(blob);
         auth_user_request->user()->credentials(Auth::Failed);
         safe_free(lm_request->server_blob);
         lm_request->releaseAuthServer();
         debugs(29, 4, HERE << "Failed validating user via NTLM. Error returned '" << blob << "'");
-    } else if (strncasecmp(reply, "BH ", 3) == 0) {
+        break;
+
+    case HelperReply::Unknown:
+        debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication Helper '" << reply.whichServer << "' crashed!.");
+        blob = "Internal error";
+        /* continue to the next case */
+
+    case HelperReply::BrokenHelper:
         /* TODO kick off a refresh process. This can occur after a YR or after
          * a KK. If after a YR release the helper and resubmit the request via
          * Authenticate NTLM start.
@@ -333,9 +341,7 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply)
         safe_free(lm_request->server_blob);
         lm_request->releaseAuthServer();
         debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication validating user. Error returned '" << reply << "'");
-    } else {
-        /* protocol error */
-        fatalf("authenticateNTLMHandleReply: *** Unsupported helper response ***, '%s'\n", reply);
+        break;
     }
 
     if (lm_request->request) {

diff --git a/src/auth/ntlm/UserRequest.h b/src/auth/ntlm/UserRequest.h
index 82e72b101d4e49593a4ee21ae64b3087f028e931..406ae5e305443378aa7d29558fead6e6182442ff 100644 (file)
--- a/src/auth/ntlm/UserRequest.h
+++ b/src/auth/ntlm/UserRequest.h
@@ -48,7 +48,7 @@ public:
     HttpRequest *request;
 
 private:
-    static HLPSCB HandleReply;
+    static HLPCB HandleReply;
 };
 
 } // namespace Ntlm

diff --git a/src/client_side.cc b/src/client_side.cc
index d3d8d7ff44e5c5a9d0a3538ec0c63e0d192ed4c2..0ec0ece14a530c18eb92a8dffa5acc038b9dd4e6 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3689,23 +3689,23 @@ httpsAccept(const CommAcceptCbParams &params)
 }
 
 void
-ConnStateData::sslCrtdHandleReplyWrapper(void *data, char *reply)
+ConnStateData::sslCrtdHandleReplyWrapper(void *data, const HelperReply &reply)
 {
     ConnStateData * state_data = (ConnStateData *)(data);
     state_data->sslCrtdHandleReply(reply);
 }
 
 void
-ConnStateData::sslCrtdHandleReply(const char * reply)
+ConnStateData::sslCrtdHandleReply(const HelperReply &reply)
 {
-    if (!reply) {
+    if (!reply.other().hasContent()) {
         debugs(1, DBG_IMPORTANT, HERE << "\"ssl_crtd\" helper return <NULL> reply");
     } else {
         Ssl::CrtdMessage reply_message;
-        if (reply_message.parse(reply, strlen(reply)) != Ssl::CrtdMessage::OK) {
+        if (reply_message.parse(reply.other().content(), reply.other().contentSize()) != Ssl::CrtdMessage::OK) {
             debugs(33, 5, HERE << "Reply from ssl_crtd for " << sslConnectHostOrIp << " is incorrect");
         } else {
-            if (reply_message.getCode() != "OK") {
+            if (reply.result != HelperReply::Okay) {
                 debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " cannot be generated. ssl_crtd response: " << reply_message.getBody());
             } else {
                 debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " was successfully recieved from ssl_crtd");

diff --git a/src/client_side.h b/src/client_side.h
index 2df5c9b19b3e45c93ade440d7937afe663ac854d..de9b94c4f9e16ecd1e3ed2eb3a48770aa1bd6c76 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -53,6 +53,7 @@ class ConnStateData;
 class ClientHttpRequest;
 class clientStreamNode;
 class ChunkedCodingParser;
+class HelperReply;
 namespace AnyP
 {
 class PortCfg;
@@ -342,9 +343,9 @@ public:
      */
     void getSslContextDone(SSL_CTX * sslContext, bool isNew = false);
     /// Callback function. It is called when squid receive message from ssl_crtd.
-    static void sslCrtdHandleReplyWrapper(void *data, char *reply);
+    static void sslCrtdHandleReplyWrapper(void *data, const HelperReply &reply);
     /// Proccess response from ssl_crtd.
-    void sslCrtdHandleReply(const char * reply);
+    void sslCrtdHandleReply(const HelperReply &reply);
 
     void switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode);
     bool switchedToHttps() const { return switchedToHttps_; }

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 4070550e8b8cf2ff2a6b844ccad4d46f059a77cf..047b672ad7f18110779b4d7dd053120d02bdbf3f 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -57,6 +57,7 @@
 #include "fde.h"
 #include "format/Token.h"
 #include "gopher.h"
+#include "helper.h"
 #include "http.h"
 #include "HttpHdrCc.h"
 #include "HttpReply.h"
@@ -67,6 +68,7 @@
 #include "MemObject.h"
 #include "Parsing.h"
 #include "profiler/Profiler.h"
+#include "redirect.h"
 #include "SquidConfig.h"
 #include "SquidTime.h"
 #include "Store.h"
@@ -129,7 +131,7 @@ static void sslBumpAccessCheckDoneWrapper(allow_t, void *);
 #endif
 static int clientHierarchical(ClientHttpRequest * http);
 static void clientInterpretRequestHeaders(ClientHttpRequest * http);
-static RH clientRedirectDoneWrapper;
+static HLPCB clientRedirectDoneWrapper;
 static void checkNoCacheDoneWrapper(allow_t, void *);
 SQUIDCEXTERN CSR clientGetMoreData;
 SQUIDCEXTERN CSS clientReplyStatus;
@@ -900,7 +902,7 @@ clientRedirectAccessCheckDone(allow_t answer, void *data)
     if (answer == ACCESS_ALLOWED)
         redirectStart(http, clientRedirectDoneWrapper, context);
     else
-        context->clientRedirectDone(NULL);
+        context->clientRedirectDone(HelperReply(NULL,0));
 }
 
 void
@@ -1181,7 +1183,7 @@ clientInterpretRequestHeaders(ClientHttpRequest * http)
 }
 
 void
-clientRedirectDoneWrapper(void *data, char *result)
+clientRedirectDoneWrapper(void *data, const HelperReply &result)
 {
     ClientRequestContext *calloutContext = (ClientRequestContext *)data;
 
@@ -1192,14 +1194,19 @@ clientRedirectDoneWrapper(void *data, char *result)
 }
 
 void
-ClientRequestContext::clientRedirectDone(char *result)
+ClientRequestContext::clientRedirectDone(const HelperReply &reply)
 {
     HttpRequest *old_request = http->request;
-    debugs(85, 5, "clientRedirectDone: '" << http->uri << "' result=" << (result ? result : "NULL"));
+    debugs(85, 5, HERE << "'" << http->uri << "' result=" << reply);
     assert(redirect_state == REDIRECT_PENDING);
     redirect_state = REDIRECT_DONE;
 
-    if (result) {
+    if (reply.other().hasContent()) {
+        /* 2012-06-28: This cast is due to urlParse() truncating too-long URLs itself.
+         * At this point altering the helper buffer in that way is not harmful, but annoying.
+         * When Bug 1961 is resolved and urlParse has a const API, this needs to die.
+         */
+        char * result = const_cast<char*>(reply.other().content());
         http_status status = (http_status) atoi(result);
 
         if (status == HTTP_MOVED_PERMANENTLY
@@ -1207,7 +1214,7 @@ ClientRequestContext::clientRedirectDone(char *result)
                 || status == HTTP_SEE_OTHER
                 || status == HTTP_PERMANENT_REDIRECT
                 || status == HTTP_TEMPORARY_REDIRECT) {
-            char *t = result;
+            char *t = NULL;
 
             if ((t = strchr(result, ':')) != NULL) {
                 http->redirect.status = status;

diff --git a/src/client_side_request.h b/src/client_side_request.h
index ca12ad247cb35e1361c49d8b7395ccc0ae27eb9d..a05177f87e43b9af2779998581ca66ce07169f1c 100644 (file)
--- a/src/client_side_request.h
+++ b/src/client_side_request.h
@@ -206,7 +206,6 @@ int clientHttpRequestStatus(int fd, ClientHttpRequest const *http);
 void clientAccessCheck(ClientHttpRequest *);
 
 /* ones that should be elsewhere */
-void redirectStart(ClientHttpRequest *, RH *, void *);
 void tunnelStart(ClientHttpRequest *, int64_t *, int *);
 
 #if _USE_INLINE_

diff --git a/src/dns.cc b/src/dns.cc
index 31b83d18a9da23b186f37e059308e6f706260abc..f1194339456ee14ac2335b6c0ef4cb06f9131830 100644 (file)
--- a/src/dns.cc
+++ b/src/dns.cc
@@ -33,6 +33,7 @@
 
 #include "squid.h"
 #include "helper.h"
+#include "HelperReply.h"
 #include "mgr/Registration.h"
 #include "SquidConfig.h"
 #include "SquidTime.h"
@@ -129,8 +130,8 @@ dnsSubmit(const char *lookup, HLPCB * callback, void *data)
 
         debugs(34, DBG_IMPORTANT, "dnsSubmit: queue overload, rejecting " << lookup);
 
-        callback(data, (char *)"$fail Temporary network problem, please retry later");
-
+        const char *t = "$fail Temporary network problem, please retry later";
+        callback(data, HelperReply(t, strlen(t)));
         return;
     }
 

diff --git a/src/external_acl.cc b/src/external_acl.cc
index b9cb7d19d841274f875386eca8c84578495e9899..4dcd91ae4a39226a734d05253a46441e35c5f1aa 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -1306,30 +1306,28 @@ free_externalAclState(void *data)
  * the whitespace escaped using \ (\ escaping obviously also applies to
  * any " characters)
  */
-
 static void
-externalAclHandleReply(void *data, char *reply)
+externalAclHandleReply(void *data, const HelperReply &reply)
 {
     externalAclState *state = static_cast<externalAclState *>(data);
     externalAclState *next;
-    char *status;
-    char *token;
-    char *value;
     char *t = NULL;
     ExternalACLEntryData entryData;
     entryData.result = ACCESS_DENIED;
     external_acl_entry *entry = NULL;
 
-    debugs(82, 2, "externalAclHandleReply: reply=\"" << reply << "\"");
+    debugs(82, 2, HERE << "reply=" << reply);
 
-    if (reply) {
-        status = strwordtok(reply, &t);
+    if (reply.result == HelperReply::Okay)
+        entryData.result = ACCESS_ALLOWED;
+    // XXX: handle other non-DENIED results better
 
-        if (status && strcmp(status, "OK") == 0)
-            entryData.result = ACCESS_ALLOWED;
+    if (reply.other().hasContent()) {
+        char *temp = reply.modifiableOther().content();
+        char *token = strwordtok(temp, &t);
 
         while ((token = strwordtok(NULL, &t))) {
-            value = strchr(token, '=');
+            char *value = strchr(token, '=');
 
             if (value) {
                 *value = '\0'; /* terminate the token, and move up to the value */
@@ -1363,7 +1361,8 @@ externalAclHandleReply(void *data, char *reply)
     dlinkDelete(&state->list, &state->def->queue);
 
     if (cbdataReferenceValid(state->def)) {
-        if (reply)
+        // only cache OK and ERR results.
+        if (reply.result == HelperReply::Okay || reply.result == HelperReply::Error)
             entry = external_acl_cache_add(state->def, state->key, entryData);
         else {
             external_acl_entry *oldentry = (external_acl_entry *)hash_lookup(state->def->cache, state->key);

diff --git a/src/fqdncache.cc b/src/fqdncache.cc
index 2530ac85520eb50b7c34cbe10220d8f36267e4bd..f341bd329c57a7a0d25d2f305979c2ff64dfed84 100644 (file)
--- a/src/fqdncache.cc
+++ b/src/fqdncache.cc
@@ -34,6 +34,7 @@
 #include "cbdata.h"
 #include "DnsLookupDetails.h"
 #include "event.h"
+#include "HelperReply.h"
 #include "Mem.h"
 #include "mgr/Registration.h"
 #include "SquidConfig.h"
@@ -497,7 +498,7 @@ fqdncacheParse(fqdncache_entry *f, const rfc1035_rr * answers, int nr, const cha
  */
 static void
 #if USE_DNSHELPER
-fqdncacheHandleReply(void *data, char *reply)
+fqdncacheHandleReply(void *data, const HelperReply &reply)
 #else
 fqdncacheHandleReply(void *data, const rfc1035_rr * answers, int na, const char *error_message)
 #endif
@@ -509,7 +510,7 @@ fqdncacheHandleReply(void *data, const rfc1035_rr * answers, int na, const char
     statCounter.dns.svcTime.count(age);
 #if USE_DNSHELPER
 
-    fqdncacheParse(f, reply);
+    fqdncacheParse(f, reply.other().content());
 #else
 
     fqdncacheParse(f, answers, na, error_message);

diff --git a/src/helper.cc b/src/helper.cc
index 85d0dc0392f3df9c87d61f02d4fdaf3835c8347c..4be084bdb157375d5cc73e2e2bffb7d6c70b1b59 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -398,7 +398,7 @@ helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data)
 {
     if (hlp == NULL) {
         debugs(84, 3, "helperSubmit: hlp == NULL");
-        callback(data, NULL);
+        callback(data, HelperReply(NULL,0));
         return;
     }
 
@@ -419,11 +419,11 @@ helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data)
 
 /// lastserver = "server last used as part of a reserved request sequence"
 void
-helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver)
+helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver)
 {
     if (hlp == NULL) {
         debugs(84, 3, "helperStatefulSubmit: hlp == NULL");
-        callback(data, 0, NULL);
+        callback(data, HelperReply(NULL,0));
         return;
     }
 
@@ -751,11 +751,12 @@ helperServerFree(helper_server *srv)
     }
 
     for (i = 0; i < concurrency; ++i) {
+        // XXX: re-schedule these on another helper?
         if ((r = srv->requests[i])) {
             void *cbdata;
 
             if (cbdataReferenceValidDone(r->data, &cbdata))
-                r->callback(cbdata, NULL);
+                r->callback(cbdata, HelperReply(NULL,0));
 
             helperRequestFree(r);
 
@@ -818,8 +819,11 @@ helperStatefulServerFree(helper_stateful_server *srv)
     if ((r = srv->request)) {
         void *cbdata;
 
-        if (cbdataReferenceValidDone(r->data, &cbdata))
-            r->callback(cbdata, srv, NULL);
+        if (cbdataReferenceValidDone(r->data, &cbdata)) {
+            HelperReply nilReply(NULL,0);
+            nilReply.whichServer = srv;
+            r->callback(cbdata, nilReply);
+        }
 
         helperStatefulRequestFree(r);
 
@@ -835,10 +839,14 @@ helperStatefulServerFree(helper_stateful_server *srv)
 }
 
 /// Calls back with a pointer to the buffer with the helper output
-static void helperReturnBuffer(int request_number, helper_server * srv, helper * hlp, char * msg, char * msg_end)
+static void
+helperReturnBuffer(int request_number, helper_server * srv, helper * hlp, char * msg, char * msg_end)
 {
     helper_request *r = srv->requests[request_number];
     if (r) {
+// TODO: parse the reply into new helper reply object
+// pass that to the callback instead of msg
+
         HLPCB *callback = r->callback;
 
         srv->requests[request_number] = NULL;
@@ -847,7 +855,7 @@ static void helperReturnBuffer(int request_number, helper_server * srv, helper *
 
         void *cbdata = NULL;
         if (cbdataReferenceValidDone(r->data, &cbdata))
-            callback(cbdata, msg);
+            callback(cbdata, HelperReply(msg, (msg_end-msg)));
 
         -- srv->stats.pending;
         ++ srv->stats.replies;
@@ -1017,7 +1025,9 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *buf, size_t
         *t = '\0';
 
         if (r && cbdataReferenceValid(r->data)) {
-            r->callback(r->data, srv, srv->rbuf);
+            HelperReply res(srv->rbuf, (t - srv->rbuf));
+            res.whichServer = srv;
+            r->callback(r->data, res);
         } else {
             debugs(84, DBG_IMPORTANT, "StatefulHandleRead: no callback data registered");
             called = 0;
@@ -1350,11 +1360,13 @@ helperStatefulDispatch(helper_stateful_server * srv, helper_stateful_request * r
         /* a callback is needed before this request can _use_ a helper. */
         /* we don't care about releasing this helper. The request NEVER
          * gets to the helper. So we throw away the return code */
-        r->callback(r->data, srv, NULL);
+        HelperReply nilReply(NULL,0);
+        nilReply.whichServer = srv;
+        r->callback(r->data, nilReply);
         /* throw away the placeholder */
         helperStatefulRequestFree(r);
         /* and push the queue. Note that the callback may have submitted a new
-         * request to the helper which is why we test for the request*/
+         * request to the helper which is why we test for the request */
 
         if (srv->request == NULL)
             helperStatefulServerDone(srv);

diff --git a/src/helper.h b/src/helper.h
index d929049c26051bfcb54d0d8f1b30fdc7ba0b43ce..1ee9d5e904b8b998efac05ec4ea88f655f521c7e 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -39,10 +39,11 @@
 #include "dlink.h"
 #include "ip/Address.h"
 #include "HelperChildConfig.h"
+#include "HelperReply.h"
 
 class helper_request;
 
-typedef void HLPSCB(void *, void *lastserver, char *buf);
+typedef void HLPCB(void *, const HelperReply &reply);
 
 class helper
 {
@@ -191,7 +192,7 @@ class helper_stateful_request
 public:
     MEMPROXY_CLASS(helper_stateful_request);
     char *buf;
-    HLPSCB *callback;
+    HLPCB *callback;
     int placeholder;           /* if 1, this is a dummy request waiting for a stateful helper to become available */
     void *data;
 };
@@ -202,7 +203,7 @@ MEMPROXY_CLASS_INLINE(helper_stateful_request);
 void helperOpenServers(helper * hlp);
 void helperStatefulOpenServers(statefulhelper * hlp);
 void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data);
-void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver);
+void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver);
 void helperStats(StoreEntry * sentry, helper * hlp, const char *label = NULL);
 void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label = NULL);
 void helperShutdown(helper * hlp);

diff --git a/src/ipcache.cc b/src/ipcache.cc
index dba1ce0734c7439b0228411125645caa7377dc6b..2d1071da714d8393846b671b245ed0053c53ad25 100644 (file)
--- a/src/ipcache.cc
+++ b/src/ipcache.cc
@@ -595,7 +595,7 @@ ipcacheParse(ipcache_entry *i, const rfc1035_rr * answers, int nr, const char *e
 /// \ingroup IPCacheInternal
 static void
 #if USE_DNSHELPER
-ipcacheHandleReply(void *data, char *reply)
+ipcacheHandleReply(void *data, const HelperReply &reply)
 #else
 ipcacheHandleReply(void *data, const rfc1035_rr * answers, int na, const char *error_message)
 #endif
@@ -607,7 +607,7 @@ ipcacheHandleReply(void *data, const rfc1035_rr * answers, int na, const char *e
     statCounter.dns.svcTime.count(age);
 
 #if USE_DNSHELPER
-    ipcacheParse(i, reply);
+    ipcacheParse(i, reply.other().content());
 #else
 
     int done = ipcacheParse(i, answers, na, error_message);

diff --git a/src/redirect.cc b/src/redirect.cc
index bb021be12a386efc8d5a0d8c7917a55229b9186a..63aadb3b5b519f76ab9beafa88fd336a24b77963 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -40,7 +40,6 @@
 #include "fqdncache.h"
 #include "globals.h"
 #include "HttpRequest.h"
-#include "helper.h"
 #include "mgr/Registration.h"
 #include "redirect.h"
 #include "rfc1738.h"
@@ -63,7 +62,7 @@ typedef struct {
     Ip::Address client_addr;
     const char *client_ident;
     const char *method_s;
-    RH *handler;
+    HLPCB *handler;
 } redirectStateData;
 
 static HLPCB redirectHandleReply;
@@ -74,21 +73,36 @@ static int n_bypassed = 0;
 CBDATA_TYPE(redirectStateData);
 
 static void
-redirectHandleReply(void *data, char *reply)
+redirectHandleReply(void *data, const HelperReply &reply)
 {
     redirectStateData *r = static_cast<redirectStateData *>(data);
-    char *t;
-    void *cbdata;
-    debugs(61, 5, "redirectHandleRead: {" << (reply && *reply != '\0' ? reply : "<NULL>") << "}");
-
-    if (reply) {
-        if ((t = strchr(reply, ' ')))
-            *t = '\0';
-
-        if (*reply == '\0')
-            reply = NULL;
+    debugs(61, 5, HERE << "reply=" << reply);
+
+    // XXX: This funtion is now kept only to check for and display this garbage use-case
+    // it can be removed when the helpers are all updated to the normalized "OK/ERR key-pairs" format
+
+    if (reply.result == HelperReply::Unknown) {
+        // BACKWARD COMPATIBILITY 2012-06-15:
+        // Some nasty old helpers send back the entire input line including extra format keys.
+        // This is especially bad for simple perl search-replace filter scripts.
+        //
+        // * trim all but the first word off the response.
+        // * warn once every 50 responses that this will stop being fixed-up soon.
+        //
+        if (const char * res = reply.other().content()) {
+            if (const char *t = strchr(res, ' ')) {
+                static int warn = 0;
+                debugs(61, (!(warn++%50)? DBG_CRITICAL:2), "UPGRADE WARNING: URL rewriter reponded with garbage '" << t <<
+                           "'. Future Squid will treat this as part of the URL.");
+                const mb_size_t garbageLength = reply.other().contentSize() - (t-res);
+                reply.modifiableOther().truncate(garbageLength);
+            }
+            if (reply.other().hasContent() && *res == '\0')
+                reply.modifiableOther().clean(); // drop the whole buffer of garbage.
+        }
     }
 
+    void *cbdata;
     if (cbdataReferenceValidDone(r->data, &cbdata))
         r->handler(cbdata, reply);
 
@@ -120,7 +134,7 @@ redirectStats(StoreEntry * sentry)
 /**** PUBLIC FUNCTIONS ****/
 
 void
-redirectStart(ClientHttpRequest * http, RH * handler, void *data)
+redirectStart(ClientHttpRequest * http, HLPCB * handler, void *data)
 {
     ConnStateData * conn = http->getConn();
     redirectStateData *r = NULL;
@@ -137,7 +151,7 @@ redirectStart(ClientHttpRequest * http, RH * handler, void *data)
     if (Config.onoff.redirector_bypass && redirectors->stats.queue_size) {
         /* Skip redirector if there is one request queued */
         ++n_bypassed;
-        handler(data, NULL);
+        handler(data, HelperReply(NULL,0));
         return;
     }
 

diff --git a/src/redirect.h b/src/redirect.h
index 6cbbdd3161e39bb164de4ed9d92b10085b157dc6..d7f73a1199710cbffc3ae53b55e65c9f57472044 100644 (file)
--- a/src/redirect.h
+++ b/src/redirect.h
@@ -33,7 +33,12 @@
  *
  */
 
+#include "helper.h"
+
+class ClientHttpRequest;
+
 void redirectInit(void);
 void redirectShutdown(void);
+void redirectStart(ClientHttpRequest *, HLPCB *, void *);
 
 #endif /* SQUID_REDIRECT_H_ */

diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc
index 6c8803b99fa9726230a1ab3d30475748f2d92a2d..d3792f819f28b906d224fa184cfda6e29e5fc768 100644 (file)
--- a/src/ssl/helper.cc
+++ b/src/ssl/helper.cc
@@ -93,7 +93,8 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void
         if (squid_curtime - first_warn > 3 * 60)
             fatal("SSL servers not responding for 3 minutes");
         debugs(34, DBG_IMPORTANT, HERE << "Queue overload, rejecting");
-        callback(data, (char *)"error 45 Temporary network problem, please retry later");
+        const char *errMsg = "BH error 45 Temporary network problem, please retry later"; // XXX: upgrade to message=""
+        callback(data, HelperReply(errMsg,strlen(errMsg)));
         return;
     }
 

diff --git a/src/tests/stub_helper.cc b/src/tests/stub_helper.cc
index d178a0396b622bc3c4e6b521eea6e647ddd9e13a..08da5eb577d0d90b19a1ad5f2770d1db734e10fa 100644 (file)
--- a/src/tests/stub_helper.cc
+++ b/src/tests/stub_helper.cc
@@ -5,7 +5,7 @@
 #include "tests/STUB.h"
 
 void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data) STUB
-void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver) STUB
+void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver) STUB
 helper::~helper() STUB
 CBDATA_CLASS_INIT(helper);
 

diff --git a/src/typedefs.h b/src/typedefs.h
index 488671adbc55d8c5865337c259c9f9c7e5ceddcb..09b1f05fa3b1bcdbeea410ce1f11d7d34c5a1b4f 100644 (file)
--- a/src/typedefs.h
+++ b/src/typedefs.h
@@ -75,7 +75,6 @@ typedef void IDCB(const char *ident, void *data);
 class CachePeer;
 typedef void IRCB(CachePeer *, peer_t, AnyP::ProtocolType, void *, void *data);
 
-typedef void RH(void *data, char *);
 /* in wordlist.h */
 
 class wordlist;
@@ -90,7 +89,6 @@ class StoreEntry;
 typedef void OBJH(StoreEntry *);
 typedef void SIGHDLR(int sig);
 typedef void STVLDCB(void *, int, int);
-typedef void HLPCB(void *, char *buf);
 typedef int HLPSAVAIL(void *);
 typedef void HLPSONEQ(void *);
 typedef void HLPCMDOPTS(int *argc, char **argv);