From: Amos Jeffries Date: Wed, 19 Sep 2012 04:45:39 +0000 (+1200) Subject: Convert helper char* response to class X-Git-Tag: SQUID_3_4_0_1~471^2~25 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0272dd08f2be7f000c031ddc1a3c720efc4696aa;p=thirdparty%2Fsquid.git Convert helper char* response to class --- diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h index f597107598..46e95e1961 100644 --- 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 index 0000000000..daf9dcf929 --- /dev/null +++ b/src/HelperReply.cc @@ -0,0 +1,96 @@ +#include "squid.h" +#include "HelperReply.h" +#include "helper.h" + +#if 0 +HelperReply::HelperReply(const HelperReply &r) : + result(r.result) + other_(r.other()), +{ +} +#endif + +HelperReply::HelperReply(const char *buf, size_t len) : + result(HelperReply::Unknown), + whichServer(NULL) +{ + // check we have something to parse + if (!buf || len < 1) + return; + + const char *p = buf; + + if (len >= 2) { + // NOTE: only increment 'p' if a result code is found. + // some helper formats (digest auth, URL-rewriter) just send a data string + // we must also check for the ' ' character after the response token here + if (!strncmp(p,"OK ",3)) { + result = HelperReply::Okay; + p+=2; + } else if (!strncmp(p,"ERR ",4)) { + result = HelperReply::Error; + p+=3; + } else if (!strncmp(p,"BH ",3)) { + result = HelperReply::BrokenHelper; + p+=2; + } else if (!strncmp(p,"TT ",3)) { + // NTLM challenge token + result = HelperReply::TT; + p+=2; + } else if (!strncmp(p,"AF ",3)) { + // NTLM OK response + result = HelperReply::AF; + p+=2; + } else if (!strncmp(p,"NA ",3)) { + // NTLM fail-closed ERR response + result = HelperReply::NA; + p+=2; + } + + for(;xisspace(*p);p++); // skip whitespace + } + + const mb_size_t blobSize = (buf+len-p); + other_.init(blobSize, 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 index 0000000000..89d352a9c3 --- /dev/null +++ b/src/HelperReply.h @@ -0,0 +1,67 @@ +#ifndef _SQUID_SRC_HELPERREPLY_H +#define _SQUID_SRC_HELPERREPLY_H + +#include "base/CbcPointer.h" +#include "MemBuf.h" + +#if HAVE_OSTREAM +#include +#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: + // implicit creation and copy are prohibited explicitly + HelperReply(); + 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); + ~HelperReply() {} + + 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(&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 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 4afc3dd211..3b72d4d397 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -354,6 +354,8 @@ squid_SOURCES = \ helper.h \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ HierarchyLogEntry.h \ $(HTCPSOURCE) \ @@ -1414,6 +1416,8 @@ tests_testCacheManager_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ $(HTCPSOURCE) \ http.cc \ HttpBody.h \ @@ -1799,6 +1803,8 @@ tests_testEvent_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ @@ -2031,6 +2037,8 @@ tests_testEventLoop_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ @@ -2260,6 +2268,8 @@ tests_test_http_range_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ @@ -2537,6 +2547,8 @@ tests_testHttpRequest_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ @@ -3580,6 +3592,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 e5b82a59a3..6847d5c016 100644 --- 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 5722f2ad09..16970418e7 100644 --- 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 300d251c36..f9bda7a6bf 100644 --- 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 a990818461..a3d9651579 100644 --- 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(data); BasicAuthQueueNode *tmpnode; - char *t = NULL; void *cbdata; - debugs(29, 5, HERE << "{" << (reply ? reply : "") << "}"); - - 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 d53c340029..a956b2da47 100644 --- 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(data); - char *t = NULL; - void *cbdata; - debugs(29, 9, HERE << "{" << (reply ? reply : "") << "}"); - - 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_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_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 ac4e71cc59..bab1f280cd 100644 --- 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(data); - char *blob, *arg = NULL; - - debugs(29, 8, HERE << "helper: '" << lastserver << "' sent us '" << (reply ? reply : "") << "'"); + 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(lastserver); + lm_request->authserver = reply.whichServer.get(); // XXX: no locking? else - assert(lm_request->authserver == lastserver); + assert(lm_request->authserver == reply.whichServer.raw()); /* 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.must_keepalive = 1; if (lm_request->request->flags.proxy_keepalive) { @@ -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 478680cfcd..79c0e19f19 100644 --- 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 bd2b4ffe13..aef9f05dcc 100644 --- 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(data); - char *blob; - debugs(29, 8, HERE << "helper: '" << lastserver << "' sent us '" << (reply ? reply : "") << "'"); + 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(lastserver); + lm_request->authserver = reply.whichServer.get(); // XXX: no locking? else - assert(lm_request->authserver == lastserver); + assert(lm_request->authserver == reply.whichServer.raw()); /* 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.must_keepalive = 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 82e72b101d..406ae5e305 100644 --- 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 84db45fc67..dcd727da46 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3678,23 +3678,23 @@ httpsAccept(const CommAcceptCbParams ¶ms) } 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 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 62a916cffa..726222501e 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -53,6 +53,7 @@ class ConnStateData; class ClientHttpRequest; class clientStreamNode; class ChunkedCodingParser; +class HelperReply; /** * Badly named. @@ -338,9 +339,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 75c80293b0..d4b67c8060 100644 --- 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 "SquidTime.h" #include "Store.h" #include "StrList.h" @@ -128,7 +130,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 *); extern "C" CSR clientGetMoreData; extern "C" CSS clientReplyStatus; @@ -899,7 +901,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 @@ -1185,7 +1187,7 @@ clientInterpretRequestHeaders(ClientHttpRequest * http) } void -clientRedirectDoneWrapper(void *data, char *result) +clientRedirectDoneWrapper(void *data, const HelperReply &result) { ClientRequestContext *calloutContext = (ClientRequestContext *)data; @@ -1196,14 +1198,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(reply.other().content()); http_status status = (http_status) atoi(result); if (status == HTTP_MOVED_PERMANENTLY @@ -1211,7 +1218,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 a296e7a135..c850e9c2a7 100644 --- a/src/client_side_request.h +++ b/src/client_side_request.h @@ -207,7 +207,6 @@ extern int clientHttpRequestStatus(int fd, ClientHttpRequest const *http); extern void clientAccessCheck(ClientHttpRequest *); /* ones that should be elsewhere */ -extern void redirectStart(ClientHttpRequest *, RH *, void *); extern void tunnelStart(ClientHttpRequest *, int64_t *, int *); #if _USE_INLINE_ diff --git a/src/dns.cc b/src/dns.cc index 43f8dd94b9..db8c86bc94 100644 --- a/src/dns.cc +++ b/src/dns.cc @@ -37,6 +37,7 @@ #include "SquidTime.h" #include "mgr/Registration.h" #include "helper.h" +#include "HelperReply.h" /* MS VisualStudio Projects are monolitich, so we need the following #if to include the external DNS code in compile process when @@ -128,8 +129,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 59ffbc19b0..a43274b643 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -1305,30 +1305,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(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 */ @@ -1362,7 +1360,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 32d284be69..09cae8d63b 100644 --- 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 "SquidDns.h" @@ -496,7 +497,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 @@ -508,7 +509,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 344c36dca2..b955ea2ebf 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -389,7 +389,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; } @@ -410,11 +410,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; } @@ -734,11 +734,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); @@ -797,8 +798,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); @@ -814,10 +818,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; @@ -826,7 +834,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; @@ -995,7 +1003,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; @@ -1324,11 +1334,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 5da5c3b752..131ab991c3 100644 --- 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 { @@ -194,7 +195,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; }; @@ -205,7 +206,7 @@ MEMPROXY_CLASS_INLINE(helper_stateful_request); SQUIDCEXTERN void helperOpenServers(helper * hlp); SQUIDCEXTERN void helperStatefulOpenServers(statefulhelper * hlp); SQUIDCEXTERN void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data); -SQUIDCEXTERN void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver); +SQUIDCEXTERN void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver); SQUIDCEXTERN void helperStats(StoreEntry * sentry, helper * hlp, const char *label = NULL); SQUIDCEXTERN void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label = NULL); SQUIDCEXTERN void helperShutdown(helper * hlp); diff --git a/src/ipcache.cc b/src/ipcache.cc index 57e5f4fd63..6477f46429 100644 --- a/src/ipcache.cc +++ b/src/ipcache.cc @@ -594,7 +594,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 @@ -606,7 +606,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 ef46b9cffc..90f9ec1e8c 100644 --- 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" @@ -62,7 +61,7 @@ typedef struct { Ip::Address client_addr; const char *client_ident; const char *method_s; - RH *handler; + HLPCB *handler; } redirectStateData; static HLPCB redirectHandleReply; @@ -73,21 +72,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(data); - char *t; - void *cbdata; - debugs(61, 5, "redirectHandleRead: {" << (reply && *reply != '\0' ? reply : "") << "}"); - - 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); @@ -119,7 +133,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; @@ -136,7 +150,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 dd74a033e3..504fb4339b 100644 --- a/src/redirect.h +++ b/src/redirect.h @@ -33,7 +33,12 @@ * */ +#include "helper.h" + +class ClientHttpRequest; + extern void redirectInit(void); extern void redirectShutdown(void); +extern void redirectStart(ClientHttpRequest *, HLPCB *, void *); #endif /* SQUID_REDIRECT_H_ */ diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc index 0f76cebf0b..323d41b61f 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -96,7 +96,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 d178a0396b..08da5eb577 100644 --- 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 8a93596141..6ed2815f99 100644 --- a/src/typedefs.h +++ b/src/typedefs.h @@ -122,7 +122,6 @@ typedef void IDCB(const char *ident, void *data); #include "anyp/ProtocolType.h" typedef void IRCB(struct peer *, peer_t, AnyP::ProtocolType, void *, void *data); -typedef void RH(void *data, char *); /* in wordlist.h */ class wordlist; @@ -137,7 +136,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);