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 b464e65eceabd68a5992156441ee74ae4fafba63..f1ab260f95cb568be7be1927c3766fba12dedafd 100644 (file)
--- 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 5774f9cf84c916f0c7f3691643c49ab7bacfe8c1..35a3a443a9d8d2a48f7dd1ceb7ad1126737b403d 100644 (file)
--- 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 beffab6a461875de617b311f77316b3607b38758..ebf2749f66fa1b782f8a0e29422d9ca96daf9cd9 100644 (file)
--- 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 47dfa34bb1a91900e5179d48ede184d083897e74..1fb72680a2117863cecfe1c7cccf9e6205b28785 100644 (file)
--- a/src/adaptation/Iterator.cc
+++ b/src/adaptation/Iterator.cc
@@ -57,6 +57,12 @@ void Adaptation::Iterator::step()
         return;
     }
 
+    HttpRequest *request = dynamic_cast<HttpRequest*>(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 bfb92f58778f365e969f9ec4ddc7a5ced21fd08e..b59ba4d71ce5f487ce9c512c58e8a6efbc8a571d 100644 (file)
--- 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 89f16d440194502b95d5308d9944f82bfcdff36f..610ee9e7e84d3f39a1cfb2fef6c1579a57427e85 100644 (file)
--- 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<HttpRequest*>(adapted.header);
+    // if no adapted request, update virgin (and inherit its properties later)
+    if (!request)
+        request = const_cast<HttpRequest*>(&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 8fb9423c2c6ad6a965e02901ac6c2b2974eba5a6..4bbe0b85735a6d78f7997d53d27cb39dede0e962 100644 (file)
--- 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 c46fb8f95fb9a15f844fdfe59bc7c1f4ae52605e..012d98db863b3f47962bc64fb15cf1c948d86fdd 100644 (file)
--- 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 f756ad29bbe942aa87b0f46bb572f89be882f81a..4f5542e04763538a511329164972f537b5618764 100644 (file)
--- 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 90c0a8d9a4b5eaf04ebe771d793e284c577b251d..30a91aa9856f29d1b8fb8029f8fd14d135b61c5b 100644 (file)
--- 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 ef1418017da69c1fbba8af24d34408f83f408c97..f23193a6b2086dc18bbfb5cbe7d472132817bb21 100644 (file)
--- 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 d4104b061cc888d2f2dd24a1d3a5c5d53cd99bb4..2861a20da2bb754a954e2a4560ea58de7c957ad9 100644 (file)
--- 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 1f2babf9976bf70cbc126d51b84e8fc68372af12..0a93966a1f801c16daf233400379da9a0b82448c 100644 (file)
--- 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 8dd4d6c114b14bd3f73c61a6ad1f892d7cde5871..885af62a1b013e9f6615ee8e7b9e36f46ebf2ea5 100644 (file)
--- 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 e560f04e68aee345e37dfd70bb8e9eb0e368a2b7..ece91283311ce604093b1e7f5fa788a9121fb632 100644 (file)
--- 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 63f78bac35fc5e354348fd9c604f29a8259f55cf..e7dca15a0b9231bcacd34da1df76e4898f90af8c 100644 (file)
--- 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 b2c74b43509344a15d9edddb73f13ad4c81f0088..0a73b9f5afa874abf25fba8d1986908ecb3ba9dc 100644 (file)
--- 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();
         }