Fix HttpHeaderStats definition to include hoErrorDetail (#787)

... when Squid is built --with-openssl.

We were "lucky" that the memory area after HttpHeaderStats was not,
apparently, used for anything important enough when HttpHeader::parse(),
indirectly called from errorInitialize() during initial Squid
configuration, was writing to it.

Detected by using AddressSanitizer.

The bug was created in commit 02259ff and cemented by commit 2673511.

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 4a7edc802b8b3f622e64ea1a4bf06a889c2ab67d..e783fe13c14a4e3bc0192bda41ba87193a155f95 100644 (file)
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -251,6 +251,7 @@ Thank you!
     Jorge Ivan Burgos Aguilar <jorgeivanburgosaguilar@gmail.com>
     Jose Luis Godoy <joseluis.godoy@correo.aeat.es>
     Jose-Marcio Martins da Cruz <Jose-Marcio.Martins@mines-paristech.fr>
+    Joshua Rogers <megamansec@gmail.com>
     Joshua Root <jmr@macports.org>
     Joshua Root <josh+squid@root.id.au>
     JPP <jpp1@frws.com>

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index 31472069dbd0cee0eb12a0ea03381e1602eecfef..bb230dd8478c9ee06dd4836cd4d698ecd50573e7 100644 (file)
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -35,6 +35,7 @@
 #include "util.h"
 
 #include <algorithm>
+#include <array>
 
 /* XXX: the whole set of API managing the entries vector should be rethought
  *      after the parse4r-ng effort is complete.
@@ -74,7 +75,7 @@ static HttpHeaderMask ReplyHeadersMask;     /* set run-time using ReplyHeaders *
 
 /* header accounting */
 // NP: keep in sync with enum http_hdr_owner_type
-static HttpHeaderStat HttpHeaderStats[] = {
+static std::array<HttpHeaderStat, hoEnd> HttpHeaderStats = {
     HttpHeaderStat(/*hoNone*/ "all", NULL),
 #if USE_HTCP
     HttpHeaderStat(/*hoHtcpReply*/ "HTCP reply", &ReplyHeadersMask),
@@ -82,11 +83,10 @@ static HttpHeaderStat HttpHeaderStats[] = {
     HttpHeaderStat(/*hoRequest*/ "request", &RequestHeadersMask),
     HttpHeaderStat(/*hoReply*/ "reply", &ReplyHeadersMask)
 #if USE_OPENSSL
-    /* hoErrorDetail */
+    , HttpHeaderStat(/*hoErrorDetail*/ "error detail templates", nullptr)
 #endif
     /* hoEnd */
 };
-static int HttpHeaderStatCount = countof(HttpHeaderStats);
 
 static int HeaderEntryParsedCount = 0;
 
@@ -129,8 +129,8 @@ httpHeaderInitModule(void)
             CBIT_SET(ReplyHeadersMask,h);
     }
 
-    /* header stats initialized by class constructor */
-    assert(HttpHeaderStatCount == hoReply + 1);
+    assert(HttpHeaderStats[0].label && "httpHeaderInitModule() called via main()");
+    assert(HttpHeaderStats[hoEnd-1].label && "HttpHeaderStats created with all elements");
 
     /* init dependent modules */
     httpHdrCcInitModule();
@@ -1639,6 +1639,9 @@ httpHeaderStatDump(const HttpHeaderStat * hs, StoreEntry * e)
     assert(hs);
     assert(e);
 
+    if (!hs->owner_mask)
+        return; // these HttpHeaderStat objects were not meant to be dumped here
+
     dump_stat = hs;
     storeAppendPrintf(e, "\nHeader Stats: %s\n", hs->label);
     storeAppendPrintf(e, "\nField type distribution\n");
@@ -1664,7 +1667,6 @@ httpHeaderStatDump(const HttpHeaderStat * hs, StoreEntry * e)
 void
 httpHeaderStoreReport(StoreEntry * e)
 {
-    int i;
     assert(e);
 
     HttpHeaderStats[0].parsedCount =
@@ -1676,9 +1678,8 @@ httpHeaderStoreReport(StoreEntry * e)
     HttpHeaderStats[0].busyDestroyedCount =
         HttpHeaderStats[hoRequest].busyDestroyedCount + HttpHeaderStats[hoReply].busyDestroyedCount;
 
-    for (i = 1; i < HttpHeaderStatCount; ++i) {
-        httpHeaderStatDump(HttpHeaderStats + i, e);
-    }
+    for (const auto &stats: HttpHeaderStats)
+        httpHeaderStatDump(&stats, e);
 
     /* field stats for all messages */
     storeAppendPrintf(e, "\nHttp Fields Stats (replies and requests)\n");