From: Christos Tsantilas Date: Fri, 20 Apr 2012 17:14:56 +0000 (+0300) Subject: Detail the error page error X-Git-Tag: BumpSslServerFirst.take08~14 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=129fe2a1bdbdd0db66964797e04c390edafeaa76;p=thirdparty%2Fsquid.git Detail the error page error This patch trying to handle at least the following three cases when we are reporting error details to the user and logging error details: 1) Shallow error: The same code discovers the error and creates the error page. The request details will be in sync with the error page details because they are discovered at the same time, by the same code. 2) Honored deep error: Somewhere deep inside, say, ICAP or DNS code, an error was detected and detailed. The error condition/answer slowly and asynchronously propagated to the place where the error page is being created. We want to preserve that original deep detail if any or provide the current high-level detail if no deep detail is available. 3) Bypassed deep error1 followed by error2: Somewhere deep inside, say, ICAP or DNS code, error1 was detected and detailed. The error1 condition started propagating up but the ICAP or DNS transaction was eventually successfully retried. Later, a deep or shallow error2 was discovered. The error1 detail becomes irrelevant when we started to retry the failed transaction. This patch: - Reset the error details when ICAP transactions retried, adaptation services retried or replaced by the fail-over service and when the forwarding code retry the connection to the destication servers. - On SSLBump errors the error details, logged in both master CONNECT request plus the first tunnelled GET request. To achieve this the error details of the bump-server-first fake request saved to the CONNECT HttpRequest object, and the logging of CONNECT request delayed until we have the bump-server-first answer (freeAllContexts called after SSL-Server answered). - Fix the cases where we set custom error codes (internal squid error codes) to ErrorState::xerrno member. This member is only for system errors. - We should not set ErrorState::xerrno to system err number unless we know that the current system error triggered the error page generation. This patch sets this member only to the system errorno passed by squid API (eg AsyncCalls API). This is also fix a possible bug in gopher.cc subsystem. - We are setting the HttpRequest:detailError inside ErrorState::BuildHttpReply method, where we have all the information required to corrently build HttpRequest error details. --- diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index b464e65ece..f1ab260f95 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -528,6 +528,14 @@ HttpRequest::detailError(err_type aType, int aDetail) errDetail = aDetail; } +void +HttpRequest::clearError() +{ + debugs(11, 7, HERE << "old error details: " << errType << '/' << errDetail); + errType = ERR_NONE; + errDetail = ERR_DETAIL_NONE; +} + const char *HttpRequest::packableURI(bool full_uri) const { if (full_uri) diff --git a/src/HttpRequest.h b/src/HttpRequest.h index 5774f9cf84..35a3a443a9 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -122,6 +122,8 @@ public: /// sets error detail if no earlier detail was available void detailError(err_type aType, int aDetail); + /// clear error details, useful for retries/repeats + void clearError(); protected: void clean(); diff --git a/src/Server.cc b/src/Server.cc index beffab6a46..ebf2749f66 100644 --- a/src/Server.cc +++ b/src/Server.cc @@ -832,7 +832,7 @@ ServerStateData::handleAdaptationAborted(bool bypassable) if (entry->isEmpty()) { debugs(11,9, HERE << "creating ICAP error entry after ICAP failure"); ErrorState *err = new ErrorState(ERR_ICAP_FAILURE, HTTP_INTERNAL_SERVER_ERROR, request); - err->xerrno = ERR_DETAIL_ICAP_RESPMOD_EARLY; + err->detailError(ERR_DETAIL_ICAP_RESPMOD_EARLY); fwd->fail(err); fwd->dontRetry(true); } else if (request) { // update logged info directly @@ -866,7 +866,7 @@ ServerStateData::handleAdaptationBlocked(const Adaptation::Answer &answer) page_id = ERR_ACCESS_DENIED; ErrorState *err = new ErrorState(page_id, HTTP_FORBIDDEN, request); - err->xerrno = ERR_DETAIL_RESPMOD_BLOCK_EARLY; + err->detailError(ERR_DETAIL_RESPMOD_BLOCK_EARLY); fwd->fail(err); fwd->dontRetry(true); @@ -905,7 +905,6 @@ void ServerStateData::sendBodyIsTooLargeError() { ErrorState *err = new ErrorState(ERR_TOO_BIG, HTTP_FORBIDDEN, request); - err->xerrno = errno; fwd->fail(err); fwd->dontRetry(true); abortTransaction("Virgin body too large."); diff --git a/src/adaptation/Iterator.cc b/src/adaptation/Iterator.cc index 47dfa34bb1..1fb72680a2 100644 --- a/src/adaptation/Iterator.cc +++ b/src/adaptation/Iterator.cc @@ -57,6 +57,12 @@ void Adaptation::Iterator::step() return; } + HttpRequest *request = dynamic_cast(theMsg); + if (!request) + request = theCause; + assert(request); + request->clearError(); + if (iterations > Adaptation::Config::service_iteration_limit) { debugs(93,DBG_CRITICAL, "Adaptation iterations limit (" << Adaptation::Config::service_iteration_limit << ") exceeded:\n" << diff --git a/src/adaptation/icap/Launcher.cc b/src/adaptation/icap/Launcher.cc index bfb92f5877..b59ba4d71c 100644 --- a/src/adaptation/icap/Launcher.cc +++ b/src/adaptation/icap/Launcher.cc @@ -43,8 +43,10 @@ void Adaptation::Icap::Launcher::launchXaction(const char *xkind) debugs(93,4, HERE << "launching " << xkind << " xaction #" << theLaunches); Adaptation::Icap::Xaction *x = createXaction(); x->attempts = theLaunches; - if (theLaunches > 1) + if (theLaunches > 1) { + x->clearError(); x->disableRetries(); + } if (theLaunches >= TheConfig.repeat_limit) x->disableRepeats("over icap_retry_limit"); theXaction = initiateAdaptation(x); diff --git a/src/adaptation/icap/ModXact.cc b/src/adaptation/icap/ModXact.cc index 89f16d4401..610ee9e7e8 100644 --- a/src/adaptation/icap/ModXact.cc +++ b/src/adaptation/icap/ModXact.cc @@ -1914,6 +1914,17 @@ void Adaptation::Icap::ModXact::detailError(int errDetail) request->detailError(ERR_ICAP_FAILURE, errDetail); } +void Adaptation::Icap::ModXact::clearError() +{ + HttpRequest *request = dynamic_cast(adapted.header); + // if no adapted request, update virgin (and inherit its properties later) + if (!request) + request = const_cast(&virginRequest()); + + if (request) + request->clearError(); +} + /* Adaptation::Icap::ModXactLauncher */ Adaptation::Icap::ModXactLauncher::ModXactLauncher(HttpMsg *virginHeader, HttpRequest *virginCause, Adaptation::ServicePointer aService): diff --git a/src/adaptation/icap/ModXact.h b/src/adaptation/icap/ModXact.h index 8fb9423c2c..4bbe0b8573 100644 --- a/src/adaptation/icap/ModXact.h +++ b/src/adaptation/icap/ModXact.h @@ -168,6 +168,7 @@ public: /// record error detail in the virgin request if possible virtual void detailError(int errDetail); + virtual void clearError(); private: virtual void start(); diff --git a/src/adaptation/icap/Xaction.h b/src/adaptation/icap/Xaction.h index c46fb8f95f..012d98db86 100644 --- a/src/adaptation/icap/Xaction.h +++ b/src/adaptation/icap/Xaction.h @@ -134,6 +134,8 @@ public: // custom exception handling and end-of-call checks virtual void callException(const std::exception &e); virtual void callEnd(); + // clear the error details on retries/repeats + virtual void clearError() {} void dnsLookupDone(const ipcache_addrs *ia); protected: diff --git a/src/client_side.cc b/src/client_side.cc index f756ad29bb..4f5542e047 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2483,6 +2483,11 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) assert (repContext); debugs(33, 5, "Connection first has failed for " << http->uri << ". Respond with an error"); repContext->setReplyToStoreEntry(sslServerBump->entry); + /*Save the original request for logging purposes*/ + if (!context->http->al.request) + context->http->al.request = HTTPMSGLOCK(http->request); + /*Get the error details from the fake request used to retrieve SSL server certificate*/ + http->request->detailError(sslServerBump->request->errType, sslServerBump->request->errDetail); context->pullData(); return true; } @@ -2516,13 +2521,11 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) ErrorState *err = new ErrorState(ERR_SECURE_CONNECT_FAIL, HTTP_SERVICE_UNAVAILABLE, request); err->src_addr = clientConnection->remote; -#ifdef EPROTO - err->xerrno = EPROTO; -#else - err->xerrno = EACCES; -#endif Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail( SQUID_X509_V_ERR_DOMAIN_MISMATCH, sslServerBump->serverCert.get(), NULL); err->detail = errDetail; + /*Save the original request for logging purposes*/ + if (!context->http->al.request) + context->http->al.request = HTTPMSGLOCK(request); repContext->setReplyToError(request->method, err); assert(context->http->out.offset == 0); context->pullData(); @@ -3757,6 +3760,10 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer void ConnStateData::getSslContextStart() { + assert(areAllContextsForThisConnection()); + freeAllContexts(); + /* careful: freeAllContexts() above frees request, host, etc. */ + if (port->generateHostCertificates) { Ssl::CertificateProperties certProperties; buildSslCertGenerationParams(certProperties); @@ -3861,13 +3868,6 @@ ConnStateData::switchToHttps(const char *host, const int port) sslConnectHostOrIp = host; sslCommonName = host; - //HTTPMSGLOCK(currentobject->http->request); - assert(areAllContextsForThisConnection()); - freeAllContexts(); - //currentobject->connIsFinished(); - - /* careful: freeAllContexts() above frees request, host, etc. */ - // We are going to read new request flags.readMore = true; debugs(33, 5, HERE << "converting " << clientConnection << " to SSL"); @@ -3924,6 +3924,10 @@ ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection) sslCommonName = sslConnectHostOrIp; else if (sslServerBump->serverCert.get()) sslCommonName = Ssl::CommonHostName(sslServerBump->serverCert.get()); + + // copy error detail from bump-server-first request to CONNECT request + if (currentobject != NULL && currentobject->http != NULL && currentobject->http->request) + currentobject->http->request->detailError(sslServerBump->request->errType, sslServerBump->request->errDetail); } getSslContextStart(); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 90c0a8d9a4..30a91aa985 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1889,7 +1889,7 @@ ClientHttpRequest::handleAdaptationFailure(int errDetail, bool bypassable) calloutContext->error->auth_user_request = c != NULL && c->auth_user_request != NULL ? c->auth_user_request : request->auth_user_request; - request->detailError(ERR_ICAP_FAILURE, errDetail); + calloutContext->error->detailError(errDetail); calloutContext->readNextRequest = true; c->expectNoForwarding(); doCallouts(); diff --git a/src/errorpage.cc b/src/errorpage.cc index ef1418017d..f23193a6b2 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -579,10 +579,11 @@ ErrorState::ErrorState(err_type t, http_status status, HttpRequest * req) : callback(NULL), callback_data(NULL), request_hdrs(NULL), - err_msg(NULL) + err_msg(NULL), #if USE_SSL - , detail(NULL) + detail(NULL), #endif + detailCode(ERR_DETAIL_NONE) { memset(&flags, 0, sizeof(flags)); memset(&ftp, 0, sizeof(ftp)); @@ -593,10 +594,15 @@ ErrorState::ErrorState(err_type t, http_status status, HttpRequest * req) : if (req != NULL) { request = HTTPMSGLOCK(req); src_addr = req->client_addr; - request->detailError(type, ERR_DETAIL_NONE); } } +void +ErrorState::detailError(int detailCode) +{ + detailCode = detailCode; +} + void errorAppendEntry(StoreEntry * entry, ErrorState * err) { @@ -644,13 +650,6 @@ errorSend(const Comm::ConnectionPointer &conn, ErrorState * err) HttpReply *rep; debugs(4, 3, HERE << conn << ", err=" << err); assert(Comm::IsConnOpen(conn)); - /* - * ugh, this is how we make sure error codes get back to - * the client side for logging and error tracking. - */ - - if (err->request) - err->request->detailError(err->type, err->xerrno); /* moved in front of errorBuildBuf @?@ */ err->flags.flag_cbdata = 1; @@ -1220,6 +1219,21 @@ ErrorState::BuildHttpReply() /* do not memBufClean() or delete the content, it was absorbed by httpBody */ } + /* Make sure error codes get back to the client side for logging and error tracking */ + if (request) { + int edc = ERR_DETAIL_NONE; // error detail code +#if USE_SSL + if (detail) + edc = detail->errorNo(); + else +#endif + if (detailCode) + edc = detailCode; + else + edc = xerrno; + request->detailError(type, edc); + } + return rep; } diff --git a/src/errorpage.h b/src/errorpage.h index d4104b061c..2861a20da2 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -40,6 +40,7 @@ #endif #include "cbdata.h" #include "comm/forward.h" +#include "err_detail_type.h" #include "ip/Address.h" #include "MemBuf.h" #if USE_SSL @@ -104,6 +105,11 @@ public: */ HttpReply *BuildHttpReply(void); + /** + * Sets the error details + */ + void detailError(int detailCode); + private: /** * Locates error page template to be used for this error @@ -182,6 +188,9 @@ public: #if USE_SSL Ssl::ErrorDetail *detail; #endif + /// type-specific detail about the transaction error; + /// overwrites xerrno; overwritten by detail, if any. + int detailCode; private: CBDATA_CLASS2(ErrorState); }; diff --git a/src/forward.cc b/src/forward.cc index 1f2babf997..0a93966a1f 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -324,13 +324,13 @@ FwdState::startConnectionOrFail() // this server link regardless of what happens when connecting to it. // IF sucessfuly connected this top destination will become the serverConnection(). request->hier.note(serverDestinations[0], request->GetHost()); + request->clearError(); connectStart(); } else { debugs(17, 3, HERE << "Connection failed: " << entry->url()); if (!err) { ErrorState *anErr = new ErrorState(ERR_CANNOT_FORWARD, HTTP_INTERNAL_SERVER_ERROR, request); - anErr->xerrno = errno; fail(anErr); } // else use actual error from last connection attempt #if USE_SSL @@ -360,13 +360,6 @@ FwdState::fail(ErrorState * errorState) debugs(17, 5, HERE << "pconn race happened"); pconnRace = raceHappened; } - -#if USE_SSL - if (errorState->type == ERR_SECURE_CONNECT_FAIL && errorState->detail) - request->detailError(errorState->type, errorState->detail->errorNo()); - else -#endif - request->detailError(errorState->type, errorState->xerrno); } /** @@ -746,7 +739,7 @@ FwdState::initiateSSL() if ((ssl = SSL_new(sslContext)) == NULL) { debugs(83, 1, "fwdInitiateSSL: Error allocating handle: " << ERR_error_string(ERR_get_error(), NULL) ); ErrorState *anErr = new ErrorState(ERR_SOCKET_FAILURE, HTTP_INTERNAL_SERVER_ERROR, request); - anErr->xerrno = errno; + // TODO: create Ssl::ErrorDetail with OpenSSL-supplied error code fail(anErr); self = NULL; // refcounted return; diff --git a/src/ftp.cc b/src/ftp.cc index 8dd4d6c114..885af62a1b 100644 --- a/src/ftp.cc +++ b/src/ftp.cc @@ -3580,9 +3580,6 @@ ftpSendReply(FtpStateData * ftpState) http_code = HTTP_INTERNAL_SERVER_ERROR; } - if (ftpState->request) - ftpState->request->detailError(err_code, code); - ErrorState err(err_code, http_code, ftpState->request); if (ftpState->old_request) @@ -3597,6 +3594,9 @@ ftpSendReply(FtpStateData * ftpState) else err.ftp.reply = xstrdup(""); + // TODO: interpret as FTP-specific error code + err.detailError(code); + ftpState->entry->replaceHttpReply( err.BuildHttpReply() ); ftpSendQuit(ftpState); diff --git a/src/gopher.cc b/src/gopher.cc index e560f04e68..ece9128331 100644 --- a/src/gopher.cc +++ b/src/gopher.cc @@ -758,7 +758,6 @@ gopherReadReply(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm return; } - errno = 0; #if USE_DELAY_POOLS read_sz = delayId.bytesWanted(1, read_sz); #endif @@ -796,13 +795,13 @@ gopherReadReply(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm if (flag != COMM_OK) { debugs(50, 1, "gopherReadReply: error reading: " << xstrerror()); - if (ignoreErrno(errno)) { + if (ignoreErrno(xerrno)) { AsyncCall::Pointer call = commCbCall(5,4, "gopherReadReply", CommIoCbPtrFun(gopherReadReply, gopherState)); comm_read(conn, buf, read_sz, call); } else { ErrorState *err = new ErrorState(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR, gopherState->fwd->request); - err->xerrno = errno; + err->xerrno = xerrno; gopherState->fwd->fail(err); gopherState->serverConn->close(); } @@ -852,7 +851,7 @@ gopherSendComplete(const Comm::ConnectionPointer &conn, char *buf, size_t size, if (errflag) { ErrorState *err; err = new ErrorState(ERR_WRITE_ERROR, HTTP_SERVICE_UNAVAILABLE, gopherState->fwd->request); - err->xerrno = errno; + err->xerrno = xerrno; err->port = gopherState->fwd->request->port; err->url = xstrdup(entry->url()); gopherState->fwd->fail(err); diff --git a/src/http.cc b/src/http.cc index 63f78bac35..e7dca15a0b 100644 --- a/src/http.cc +++ b/src/http.cc @@ -2288,7 +2288,7 @@ HttpStateData::handleRequestBodyProducerAborted() // should not matter because either client-side will provide its own or // there will be no response at all (e.g., if the the client has left). ErrorState *err = new ErrorState(ERR_ICAP_FAILURE, HTTP_INTERNAL_SERVER_ERROR, fwd->request); - err->xerrno = ERR_DETAIL_SRV_REQMOD_REQ_BODY; + err->detailError(ERR_DETAIL_SRV_REQMOD_REQ_BODY); fwd->fail(err); } diff --git a/src/whois.cc b/src/whois.cc index b2c74b4350..0a73b9f5af 100644 --- a/src/whois.cc +++ b/src/whois.cc @@ -159,7 +159,7 @@ WhoisState::readReply(const Comm::ConnectionPointer &conn, char *aBuffer, size_t comm_read(conn, aBuffer, BUFSIZ, call); } else { ErrorState *err = new ErrorState(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR, fwd->request); - err->xerrno = errno; + err->xerrno = xerrno; fwd->fail(err); conn->close(); }