Author: Alex Rousskov <rousskov@measurement-factory.com>

Bug #1688: assertion when Squid filters HTTP headers

HttpHeader::delAt does not update the header mask. This patch adds a parameter
to delAt, which forces the caller to pay attention and call newly added
HttpHeader::refreshMask after calling HttpHeader::delAt.

The patch also optimizes Keep-Alive header handling and comments on
HDR_PROXY_CONNECTION handling.

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index 33dd9185780b665bcf3143be89d08708a206938a..d55136c56766ac47abec80b0124902bcdf314e0c 100644 (file)
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: HttpHeader.cc,v 1.130 2007/04/30 16:56:09 wessels Exp $
+ * $Id: HttpHeader.cc,v 1.131 2007/05/07 18:12:28 wessels Exp $
  *
  * DEBUG: section 55    HTTP Header
  * AUTHOR: Alex Rousskov
@@ -455,7 +455,10 @@ HttpHeader::update (HttpHeader const *fresh, HttpHeaderMask const *denied_mask)
 
         debugs(55, 7, "Updating header '" << HeadersAttrs[e->id].name << "' in cached entry");
 
-        delByName(e->name.buf());
+        if (e->id != HDR_OTHER)
+            delById(e->id);
+        else
+            delByName(e->name.buf());
 
         addEntry(e->clone());
     }
@@ -722,10 +725,9 @@ HttpHeader::delByName(const char *name)
     debugs(55, 9, "deleting '" << name << "' fields in hdr " << this);
 
     while ((e = getEntry(&pos))) {
-        if (!e->name.caseCmp(name)) {
-            delAt(pos);
-            count++;
-        } else
+        if (!e->name.caseCmp(name))
+            delAt(pos, count);
+        else
             CBIT_SET(mask, e->id);
     }
 
@@ -747,10 +749,8 @@ HttpHeader::delById(http_hdr_type id)
         return 0;
 
     while ((e = getEntry(&pos))) {
-        if (e->id == id) {
-            delAt(pos);
-            count++;
-        }
+        if (e->id == id)
+            delAt(pos, count);
     }
 
     CBIT_CLR(mask, id);
@@ -761,9 +761,11 @@ HttpHeader::delById(http_hdr_type id)
 /*
  * deletes an entry at pos and leaves a gap; leaving a gap makes it
  * possible to iterate(search) and delete fields at the same time
+ * NOTE: Does not update the header mask. Caller must follow up with
+ * a call to refreshMask() if headers_deleted was incremented.
  */
 void
-HttpHeader::delAt(HttpHeaderPos pos)
+HttpHeader::delAt(HttpHeaderPos pos, int &headers_deleted)
 {
     HttpHeaderEntry *e;
     assert(pos >= HttpHeaderInitPos && pos < (ssize_t)entries.count);
@@ -773,8 +775,22 @@ HttpHeader::delAt(HttpHeaderPos pos)
     len -= e->name.size() + 2 + e->value.size() + 2;
     assert(len >= 0);
     delete e;
+    ++headers_deleted;
 }
 
+/*
+ * Refreshes the header mask. Required after delAt() calls.
+ */
+void
+HttpHeader::refreshMask()
+{
+    httpHeaderMaskInit(&mask, 0);
+    debugs(55, 7, "refreshing the mask in hdr " << this);
+    HttpHeaderPos pos = HttpHeaderInitPos;
+    while (HttpHeaderEntry *e = getEntry(&pos)) {
+       CBIT_SET(mask, e->id);
+    }
+}
 
 /* appends an entry;
  * does not call e->clone() so one should not reuse "*e"
@@ -1712,10 +1728,13 @@ HttpHeader::removeConnectionHeaderEntries()
          * from strConnection first?
          */
 
+        int headers_deleted = 0;
         while ((e = getEntry(&pos))) {
             if (strListIsMember(&strConnection, e->name.buf(), ','))
-                delAt(pos);
+                delAt(pos, headers_deleted);
         }
+        if (headers_deleted)
+            refreshMask();
 
         delById(HDR_CONNECTION);
     }

diff --git a/src/HttpHeader.h b/src/HttpHeader.h
index 35aae1d27664c8f2a64648e23eb26dedcf0191d4..6528041043b4035fa6a9e07e6222c4b934c44d7e 100644 (file)
--- a/src/HttpHeader.h
+++ b/src/HttpHeader.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: HttpHeader.h,v 1.18 2006/10/02 09:52:06 adrian Exp $
+ * $Id: HttpHeader.h,v 1.19 2007/05/07 18:12:28 wessels Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -207,7 +207,8 @@ public:
     HttpHeaderEntry *findEntry(http_hdr_type id) const;
     int delByName(const char *name);
     int delById(http_hdr_type id);
-    void delAt(HttpHeaderPos pos);
+    void delAt(HttpHeaderPos pos, int &headers_deleted);
+    void refreshMask();
     void addEntry(HttpHeaderEntry * e);
     void insertEntry(HttpHeaderEntry * e);
     String getList(http_hdr_type id) const;

diff --git a/src/HttpHeaderTools.cc b/src/HttpHeaderTools.cc
index b15563f9a05f4edf5e301869cf4cfff8d473379e..defcf5f0b03f74ecd21205c2608bbef80f6402e3 100644 (file)
--- a/src/HttpHeaderTools.cc
+++ b/src/HttpHeaderTools.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: HttpHeaderTools.cc,v 1.58 2007/04/28 22:26:37 hno Exp $
+ * $Id: HttpHeaderTools.cc,v 1.59 2007/05/07 18:12:28 wessels Exp $
  *
  * DEBUG: section 66    HTTP Header Tools
  * AUTHOR: Alex Rousskov
@@ -425,9 +425,13 @@ httpHdrMangleList(HttpHeader * l, HttpRequest * request, int req_or_rep)
     HttpHeaderEntry *e;
     HttpHeaderPos p = HttpHeaderInitPos;
 
+    int headers_deleted = 0;
     while ((e = l->getEntry(&p)))
         if (0 == httpHdrMangle(e, request, req_or_rep))
-            l->delAt(p);
+            l->delAt(p, headers_deleted);
+
+    if (headers_deleted)
+        l->refreshMask();
 }
 
 /*

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 78726d80e51c8803657695346ed4349232ad3d72..f0fea724118161c72d2463bdf61f33e1d78017c5 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side_reply.cc,v 1.122 2007/04/30 16:56:09 wessels Exp $
+ * $Id: client_side_reply.cc,v 1.123 2007/05/07 18:12:28 wessels Exp $
  *
  * DEBUG: section 88    Client-side Reply Routines
  * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c)
@@ -1187,9 +1187,14 @@ clientReplyContext::buildReplyHeader()
     hdr->delById(HDR_ETAG);
 #endif
 
+    // TODO: Should ESIInclude.cc that calls removeConnectionHeaderEntries
+    // also delete HDR_PROXY_CONNECTION and HDR_KEEP_ALIVE like we do below?
+
+    // XXX: Should HDR_PROXY_CONNECTION by studied instead of HDR_CONNECTION?
+    // httpHeaderHasConnDir does that but we do not. Is this is a bug?
     hdr->delById(HDR_PROXY_CONNECTION);
     /* here: Keep-Alive is a field-name, not a connection directive! */
-    hdr->delByName("Keep-Alive");
+    hdr->delById(HDR_KEEP_ALIVE);
     /* remove Set-Cookie if a hit */
 
     if (is_hit)
@@ -1261,6 +1266,7 @@ clientReplyContext::buildReplyHeader()
         HttpHeaderPos pos = HttpHeaderInitPos;
         HttpHeaderEntry *e;
 
+        int headers_deleted = 0;
         while ((e = hdr->getEntry(&pos))) {
             if (e->id == HDR_WWW_AUTHENTICATE || e->id == HDR_PROXY_AUTHENTICATE) {
                 const char *value = e->value.buf();
@@ -1270,9 +1276,11 @@ clientReplyContext::buildReplyHeader()
                         ||
                         (strncasecmp(value, "Negotiate", 9) == 0 &&
                          (value[9] == '\0' || value[9] == ' ')))
-                    hdr->delAt(pos);
+                            hdr->delAt(pos, headers_deleted);
             }
         }
+        if (headers_deleted)
+            hdr->refreshMask();
     }
 
     /* Handle authentication headers */