From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com>
Date: Mon, 8 Apr 2024 18:50:51 +0000 (+0000)
Subject: NoNewGlobals for HttpHdrCc:ccLookupTable (#1750)
X-Git-Tag: SQUID_7_0_1~145
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e3aef579a39219309993d6ae24014b929b36aee1;p=thirdparty%2Fsquid.git

NoNewGlobals for HttpHdrCc:ccLookupTable (#1750)

Detected by Coverity. CID 1554655: Initialization or destruction
ordering is unspecified (GLOBAL_INIT_ORDER).

Also switched to compile-time checks for table initialization records.
---

diff --git a/src/HttpHdrCc.cc b/src/HttpHdrCc.cc
index b95b133e61..2bd9303208 100644
--- a/src/HttpHdrCc.cc
+++ b/src/HttpHdrCc.cc
@@ -9,6 +9,7 @@
 /* DEBUG: section 65    HTTP Cache Control Header */
 
 #include "squid.h"
+#include "base/EnumIterator.h"
 #include "base/LookupTable.h"
 #include "HttpHdrCc.h"
 #include "HttpHeader.h"
@@ -16,6 +17,7 @@
 #include "HttpHeaderStat.h"
 #include "HttpHeaderTools.h"
 #include "sbuf/SBuf.h"
+#include "SquidMath.h"
 #include "StatHist.h"
 #include "Store.h"
 #include "StrList.h"
@@ -23,10 +25,10 @@
 
 #include <map>
 #include <vector>
+#include <optional>
 #include <ostream>
 
-// invariant: row[j].id == j
-static LookupTable<HttpHdrCcType>::Record CcAttrs[] = {
+constexpr LookupTable<HttpHdrCcType>::Record attrsList[] = {
     {"public", HttpHdrCcType::CC_PUBLIC},
     {"private", HttpHdrCcType::CC_PRIVATE},
     {"no-cache", HttpHdrCcType::CC_NO_CACHE},
@@ -44,7 +46,40 @@ static LookupTable<HttpHdrCcType>::Record CcAttrs[] = {
     {"Other,", HttpHdrCcType::CC_OTHER}, /* ',' will protect from matches */
     {nullptr, HttpHdrCcType::CC_ENUM_END}
 };
-LookupTable<HttpHdrCcType> ccLookupTable(HttpHdrCcType::CC_OTHER,CcAttrs);
+
+constexpr const auto &
+CcAttrs() {
+    // TODO: Move these compile-time checks into LookupTable
+    ConstexprForEnum<HttpHdrCcType::CC_PUBLIC, HttpHdrCcType::CC_ENUM_END>([](const auto ev) {
+        const auto idx = static_cast<std::underlying_type<HttpHdrCcType>::type>(ev);
+        // invariant: each row has a name except the last one
+        static_assert(!attrsList[idx].name == (ev == HttpHdrCcType::CC_ENUM_END));
+        // invariant: row[idx].id == idx
+        static_assert(attrsList[idx].id == ev);
+    });
+    return attrsList;
+}
+
+static auto
+ccTypeByName(const SBuf &name) {
+    const static auto table = new LookupTable<HttpHdrCcType>(HttpHdrCcType::CC_OTHER, CcAttrs());
+    return table->lookup(name);
+}
+
+/// Safely converts an integer into a Cache-Control directive name.
+/// \returns std::nullopt if the given integer is not a valid index of a named attrsList entry
+template <typename RawId>
+static std::optional<const char *>
+ccNameByType(const RawId rawId)
+{
+    // TODO: Store a by-ID index in (and move this functionality into) LookupTable.
+    if (!Less(rawId, 0) && Less(rawId, int(HttpHdrCcType::CC_ENUM_END))) {
+        const auto idx = static_cast<std::underlying_type<HttpHdrCcType>::type>(rawId);
+        return CcAttrs()[idx].name;
+    }
+    return std::nullopt;
+}
+
 std::vector<HttpHeaderFieldStat> ccHeaderStats(HttpHdrCcType::CC_ENUM_END);
 
 /// used to walk a table of http_header_cc_type structs
@@ -56,16 +91,6 @@ operator++ (HttpHdrCcType &aHeader)
     return aHeader;
 }
 
-/// Module initialization hook
-void
-httpHdrCcInitModule(void)
-{
-    // check invariant on initialization table
-    for (unsigned int j = 0; CcAttrs[j].name != nullptr; ++j) {
-        assert(static_cast<decltype(j)>(CcAttrs[j].id) == j);
-    }
-}
-
 void
 HttpHdrCc::clear()
 {
@@ -113,7 +138,7 @@ HttpHdrCc::parse(const String & str)
         }
 
         /* find type */
-        const HttpHdrCcType type = ccLookupTable.lookup(SBuf(item,nlen));
+        const auto type = ccTypeByName(SBuf(item, nlen));
 
         // ignore known duplicate directives
         if (isSet(type)) {
@@ -258,7 +283,7 @@ HttpHdrCc::packInto(Packable * p) const
         if (isSet(flag) && flag != HttpHdrCcType::CC_OTHER) {
 
             /* print option name for all options */
-            p->appendf((pcount ? ", %s": "%s"), CcAttrs[flag].name);
+            p->appendf((pcount ? ", %s": "%s"), *ccNameByType(flag));
 
             /* for all options having values, "=value" after the name */
             switch (flag) {
@@ -332,22 +357,16 @@ httpHdrCcStatDumper(StoreEntry * sentry, int, double val, double, int count)
 {
     extern const HttpHeaderStat *dump_stat; /* argh! */
     const int id = static_cast<int>(val);
-    const bool valid_id = id >= 0 && id < static_cast<int>(HttpHdrCcType::CC_ENUM_END);
-    const char *name = valid_id ? CcAttrs[id].name : "INVALID";
-
-    if (count || valid_id)
+    const auto name = ccNameByType(id);
+    if (count || name)
         storeAppendPrintf(sentry, "%2d\t %-20s\t %5d\t %6.2f\n",
-                          id, name, count, xdiv(count, dump_stat->ccParsedCount));
+                          id, name.value_or("INVALID"), count, xdiv(count, dump_stat->ccParsedCount));
 }
 
 std::ostream &
 operator<< (std::ostream &s, HttpHdrCcType c)
 {
-    const unsigned char ic = static_cast<int>(c);
-    if (c < HttpHdrCcType::CC_ENUM_END)
-        s << CcAttrs[ic].name << '[' << ic << ']' ;
-    else
-        s << "*invalid hdrcc* [" << ic << ']';
+    s << ccNameByType(c).value_or("INVALID") << '[' << static_cast<int>(c) << ']';
     return s;
 }
 
diff --git a/src/HttpHdrCc.h b/src/HttpHdrCc.h
index b192b59ec6..7feec5a58d 100644
--- a/src/HttpHdrCc.h
+++ b/src/HttpHdrCc.h
@@ -210,7 +210,6 @@ public:
 class StatHist;
 class StoreEntry;
 
-void httpHdrCcInitModule(void);
 void httpHdrCcUpdateStats(const HttpHdrCc * cc, StatHist * hist);
 void httpHdrCcStatDumper(StoreEntry * sentry, int idx, double val, double size, int count);
 
diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index 1a52b992c3..a251328c3d 100644
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -135,7 +135,6 @@ httpHeaderInitModule(void)
     assert(HttpHeaderStats[hoEnd-1].label && "HttpHeaderStats created with all elements");
 
     /* init dependent modules */
-    httpHdrCcInitModule();
     httpHdrScInitModule();
 
     httpHeaderRegisterWithCacheManager();
diff --git a/src/base/EnumIterator.h b/src/base/EnumIterator.h
index 134b76f293..02b3e13196 100644
--- a/src/base/EnumIterator.h
+++ b/src/base/EnumIterator.h
@@ -224,5 +224,20 @@ public:
     WholeEnum() : EnumRangeT<EnumType>(EnumType::enumBegin_, EnumType::enumEnd_) {}
 };
 
+/// Compile-time iteration of sequential enum values from First to Last,
+/// inclusive. The given function is called once on each iteration, with the
+/// current enum value as an argument.
+template <auto First, auto Last, class F>
+constexpr void
+ConstexprForEnum(F &&f)
+{
+    using enumType = decltype(First);
+    using underlyingType = typename std::underlying_type<enumType>::type;
+    // std::integral_constant trick makes f(First) argument a constexpr
+    f(std::integral_constant<enumType, First>()); // including when First is Last
+    if constexpr (First < Last)
+        ConstexprForEnum<enumType(static_cast<underlyingType>(First) + 1), Last>(f);
+}
+
 #endif /* SQUID_SRC_BASE_ENUMITERATOR_H */