Make Squid more robust in the event of unexpected ICAP server responses,

in particular RESPMOD responses that don't have a res-hdr section,
or a res-body.

diff --git a/src/ICAP/ICAPClientRespmodPrecache.cc b/src/ICAP/ICAPClientRespmodPrecache.cc
index 4cc837057389a36b4b1bcdc4f79c17227f83c6f0..50e2ffac963ce8621dc019c2b154796f1c3c423c 100644 (file)
--- a/src/ICAP/ICAPClientRespmodPrecache.cc
+++ b/src/ICAP/ICAPClientRespmodPrecache.cc
@@ -128,6 +128,11 @@ void ICAPClientRespmodPrecache::noteSourceStart(MsgPipe *p)
     debug(93,5)("ICAPClientRespmodPrecache::noteSourceStart() called\n");
 
     HttpReply *reply = dynamic_cast<HttpReply*>(adapted->data->header);
+    /*
+     * The ICAP reply MUST have a new HTTP reply header, or else
+     * it is an invalid ICAP message.  Invalid ICAP messages should
+     * be handled prior to this point.
+     */
     assert(reply); // check that ICAP xaction created the right object
     httpState->takeAdaptedHeaders(reply);
 

diff --git a/src/ICAP/ICAPModXact.cc b/src/ICAP/ICAPModXact.cc
index 0632f2d614bfe9ac58d4f8a93aab47820fe06485..e6faadfbd7444f70377108719dc8d60f4d3c96b8 100644 (file)
--- a/src/ICAP/ICAPModXact.cc
+++ b/src/ICAP/ICAPModXact.cc
@@ -586,7 +586,13 @@ void ICAPModXact::parseIcapHead()
         break;
 
     case 200:
-        handle200Ok();
+
+        if (!validate200Ok()) {
+            throw TexcHere("Invalid ICAP Response");
+        } else {
+            handle200Ok();
+        }
+
         break;
 
     case 204:
@@ -594,6 +600,7 @@ void ICAPModXact::parseIcapHead()
         break;
 
     default:
+        debugs(93, 5, HERE << "ICAP status " << icapReply->sline.status);
         handleUnknownScode();
         break;
     }
@@ -606,6 +613,25 @@ void ICAPModXact::parseIcapHead()
     // TODO: Consider applying a Squid 2.5 patch to recognize 201 responses
 }
 
+bool ICAPModXact::validate200Ok()
+{
+    if (ICAP::methodRespmod == service().method) {
+        if (!gotEncapsulated("res-hdr"))
+            return false;
+
+        return true;
+    }
+
+    if (ICAP::methodReqmod == service().method) {
+        if (!gotEncapsulated("res-hdr") && !gotEncapsulated("req-hdr"))
+            return false;
+
+        return true;
+    }
+
+    return false;
+}
+
 void ICAPModXact::handle100Continue()
 {
     Must(state.writing == State::writingPaused);
@@ -728,7 +754,7 @@ void ICAPModXact::parseBody()
         if (!parsePresentBody()) // need more body data
             return;
     } else {
-        debugs(93, 5, "not expecting a body");
+        debugs(93, 5, HERE << "not expecting a body");
     }
 
     stopParsing();

diff --git a/src/ICAP/ICAPModXact.h b/src/ICAP/ICAPModXact.h
index a0f079387066fd4fc8ac7441f480640aafef06e5..ad62c85f8e242cf9b5ea4c33b5c82b5b121e7012 100644 (file)
--- a/src/ICAP/ICAPModXact.h
+++ b/src/ICAP/ICAPModXact.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: ICAPModXact.h,v 1.3 2005/12/22 22:26:31 wessels Exp $
+ * $Id: ICAPModXact.h,v 1.4 2005/12/22 23:09:09 wessels Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -194,6 +194,7 @@ private:
     void maybeAllocateHttpMsg();
 
     void handle100Continue();
+    bool validate200Ok();
     void handle200Ok();
     void handle204NoContent();
     void handleUnknownScode();

diff --git a/src/http.cc b/src/http.cc
index 93217a8265821b87045aa3f967c377bd8ce75393..038b5a327b0d431d190167ba3b3a49d2ff8585de 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: http.cc,v 1.472 2005/12/13 23:37:39 wessels Exp $
+ * $Id: http.cc,v 1.473 2005/12/22 23:09:09 wessels Exp $
  *
  * DEBUG: section 11    Hypertext Transfer Protocol (HTTP)
  * AUTHOR: Harvest Derived
@@ -2073,6 +2073,7 @@ HttpStateData::takeAdaptedHeaders(HttpReply *rep)
         return;
     }
 
+    assert (rep);
     storeEntryReplaceObject(entry, rep);
 
     /*
@@ -2109,6 +2110,12 @@ HttpStateData::doneAdapting()
 {
     debug(11,5)("HttpStateData::doneAdapting() called\n");
 
+    /*
+     * ICAP is done, so we don't need this any more.
+     */
+    delete icap;
+    cbdataReferenceDone(icap);
+
     if (!entry->isAccepting()) {
         debug(11,5)("\toops, entry is not Accepting!\n");
         icap->ownerAbort();
@@ -2116,8 +2123,10 @@ HttpStateData::doneAdapting()
         fwdComplete(fwd);
     }
 
-    assert(fd == -1);
-    httpStateFree(-1, this);
+    if (fd >= 0)
+        comm_close(fd);
+    else
+        httpStateFree(fd, this);
 }
 
 void
@@ -2125,21 +2134,32 @@ HttpStateData::abortAdapting()
 {
     debug(11,5)("HttpStateData::abortAdapting() called\n");
 
+    /*
+     * ICAP has given up, we're done with it too
+     */
+    delete icap;
+    cbdataReferenceDone(icap);
+
     if (entry->isEmpty()) {
         ErrorState *err;
         err = errorCon(ERR_ICAP_FAILURE, HTTP_INTERNAL_SERVER_ERROR);
         err->request = requestLink((HttpRequest *) request);
         err->xerrno = errno;
         fwdFail(fwd, err);
+        fwd->flags.dont_retry = 1;
         flags.do_next_read = 0;
+
+        if (fd >= 0) {
+            comm_close(fd);
+        } else {
+            fwdComplete(fwd);
+            httpStateFree(-1, this);   // deletes this
+        }
+
+        return;
     }
 
-    fwdComplete(fwd);
 
-    if (fd >= 0)
-        comm_close(fd);
-    else
-        httpStateFree(fd, this);
 }
 
 #endif