Remove dlink from HttpHdrSc (#589)

Resolves Coverity issue 1461128 Resource leak. Which is a false positive
due to analysis inability to track dlink pointers.

diff --git a/src/HttpHdrSc.cc b/src/HttpHdrSc.cc
index a30498230dbbad048c23114af69534d69772f181..9baf1a31b2df46c871424bacc9f84a967fa2ea44 100644 (file)
--- a/src/HttpHdrSc.cc
+++ b/src/HttpHdrSc.cc
@@ -124,8 +124,10 @@ HttpHdrSc::parse(const String * str)
         sct = sc->findTarget(target);
 
         if (!sct) {
-            sct = new HttpHdrScTarget(target);
-            addTarget(sct);
+            // XXX: if parse is left-to-right over field-value this should be emplace_back()
+            // currently placing on the front reverses the order of headers passed on downstream.
+            targets.emplace_front(target);
+            sct = &targets.front();
         }
 
         safe_free (temp);
@@ -189,34 +191,10 @@ HttpHdrSc::parse(const String * str)
         }
     }
 
-    return sc->targets.head != NULL;
-}
-
-HttpHdrSc::~HttpHdrSc()
-{
-    if (targets.head) {
-        dlink_node *sct = targets.head;
-
-        while (sct) {
-            HttpHdrScTarget *t = static_cast<HttpHdrScTarget *>(sct->data);
-            sct = sct->next;
-            dlinkDelete (&t->node, &targets);
-            delete t;
-        }
-    }
-}
-
-HttpHdrSc::HttpHdrSc(const HttpHdrSc &sc)
-{
-    dlink_node *node = sc.targets.head;
-
-    while (node) {
-        HttpHdrScTarget *dupsct = new HttpHdrScTarget(*static_cast<HttpHdrScTarget *>(node->data));
-        addTargetAtTail(dupsct);
-        node = node->next;
-    }
+    return !sc->targets.empty();
 }
 
+/// XXX: this function should be in HttpHdrScTarget.cc
 void
 HttpHdrScTarget::packInto(Packable * p) const
 {
@@ -249,13 +227,9 @@ HttpHdrScTarget::packInto(Packable * p) const
 void
 HttpHdrSc::packInto(Packable * p) const
 {
-    dlink_node *node;
     assert(p);
-    node = targets.head;
-
-    while (node) {
-        static_cast<HttpHdrScTarget *>(node->data)->packInto(p);
-        node = node->next;
+    for (const auto &t : targets) {
+        t.packInto(p);
     }
 }
 
@@ -266,8 +240,8 @@ HttpHdrSc::setMaxAge(char const *target, int max_age)
     HttpHdrScTarget *sct = findTarget(target);
 
     if (!sct) {
-        sct = new HttpHdrScTarget(target);
-        dlinkAddTail (sct, &sct->node, &targets);
+        targets.emplace_back(target);
+        sct = &targets.back();
     }
 
     sct->maxAge(max_age);
@@ -276,11 +250,8 @@ HttpHdrSc::setMaxAge(char const *target, int max_age)
 void
 HttpHdrSc::updateStats(StatHist * hist) const
 {
-    dlink_node *sct = targets.head;
-
-    while (sct) {
-        static_cast<HttpHdrScTarget *>(sct->data)->updateStats(hist);
-        sct = sct->next;
+    for (auto &t : targets) {
+        t.updateStats(hist);
     }
 }
 
@@ -313,18 +284,9 @@ httpHdrScStatDumper(StoreEntry * sentry, int, double val, double, int count)
 HttpHdrScTarget *
 HttpHdrSc::findTarget(const char *target)
 {
-    dlink_node *node;
-    node = targets.head;
-
-    while (node) {
-        HttpHdrScTarget *sct = (HttpHdrScTarget *)node->data;
-
-        if (target && sct->target.size() > 0 && !strcmp(target, sct->target.termedBuf()))
-            return sct;
-        else if (!target && sct->target.size() == 0)
-            return sct;
-
-        node = node->next;
+    for (auto &sct : targets) {
+        if (sct.target.cmp(target) == 0)
+            return &sct;
     }
 
     return NULL;
@@ -336,6 +298,16 @@ HttpHdrSc::getMergedTarget(const char *ourtarget)
     HttpHdrScTarget *sctus = findTarget(ourtarget);
     HttpHdrScTarget *sctgeneric = findTarget(NULL);
 
+    /* W3C Edge Architecture Specification 1.0 section 3
+     *
+     * "If more than one is targetted at a surrogate, the most specific applies.
+     *  For example,
+     *    Surrogate-Control: max-age=60, no-store;abc
+     *  The surrogate that identified itself as 'abc' would apply no-store;
+     *  others would apply max-age=60.
+     *
+     * XXX: the if statements below will *merge* the no-store and max-age settings.
+     */
     if (sctgeneric || sctus) {
         HttpHdrScTarget *sctusable = new HttpHdrScTarget(NULL);
 
@@ -350,14 +322,3 @@ HttpHdrSc::getMergedTarget(const char *ourtarget)
 
     return NULL;
 }
-
-void
-HttpHdrSc::addTarget(HttpHdrScTarget *t) {
-    dlinkAdd(t, &t->node, &targets);
-}
-
-void
-HttpHdrSc::addTargetAtTail(HttpHdrScTarget *t) {
-    dlinkAddTail (t, &t->node, &targets);
-}
-

diff --git a/src/HttpHdrSc.h b/src/HttpHdrSc.h
index d0be883e28a455dab75fe0f9694de8a3a34d014d..d2dcf60261700182e5b7958c930b008387164b73 100644 (file)
--- a/src/HttpHdrSc.h
+++ b/src/HttpHdrSc.h
@@ -9,46 +9,32 @@
 #ifndef SQUID_HTTPHDRSURROGATECONTROL_H
 #define SQUID_HTTPHDRSURROGATECONTROL_H
 
-#include "dlink.h"
-#include "mem/forward.h"
+#include "http/forward.h"
+#include "HttpHdrScTarget.h"
 #include "SquidString.h"
 
-class HttpHdrScTarget;
+#include <list>
+
 class Packable;
 class StatHist;
 class StoreEntry;
 
-typedef enum {
-    SC_NO_STORE,
-    SC_NO_STORE_REMOTE,
-    SC_MAX_AGE,
-    SC_CONTENT,
-    SC_OTHER,
-    SC_ENUM_END /* also used to mean "invalid" */
-} http_hdr_sc_type;
-
 /* http surogate control header field */
 class HttpHdrSc
 {
     MEMPROXY_CLASS(HttpHdrSc);
 
 public:
-    HttpHdrSc(const HttpHdrSc &);
-    HttpHdrSc() {}
-    ~HttpHdrSc();
-
     bool parse(const String *str);
     void packInto(Packable * p) const;
     void updateStats(StatHist *) const;
     HttpHdrScTarget * getMergedTarget(const char *ourtarget); // TODO: make const?
     void setMaxAge(char const *target, int max_age);
-    void addTarget(HttpHdrScTarget *t);
-    void addTargetAtTail(HttpHdrScTarget *t);
 
-    dlink_list targets;
 private:
     HttpHdrScTarget * findTarget (const char *target);
 
+    std::list<HttpHdrScTarget, PoolingAllocator<HttpHdrScTarget>> targets;
 };
 
 /* Http Surrogate Control Header Field */

diff --git a/src/HttpHdrScTarget.h b/src/HttpHdrScTarget.h
index bb55ccadb86bb1d54751a5eb7bccae1f4239c56d..b227009431c50cf15cf4556092524081f4d11671 100644 (file)
--- a/src/HttpHdrScTarget.h
+++ b/src/HttpHdrScTarget.h
@@ -10,7 +10,8 @@
 #define SQUID_HTTPHDRSURROGATECONTROLTARGET_H
 
 #include "defines.h" //for bit mask operations
-#include "HttpHdrSc.h"
+#include "http/forward.h"
+#include "SquidString.h"
 
 class Packable;
 class StatHist;
@@ -22,21 +23,16 @@ class StoreEntry;
  */
 class HttpHdrScTarget
 {
-    MEMPROXY_CLASS(HttpHdrScTarget);
-
     // parsing is done in HttpHdrSc, need to grant them access.
     friend class HttpHdrSc;
 public:
     static const int MAX_AGE_UNSET=-1; //max-age is unset
     static const int MAX_STALE_UNSET=0; //max-stale is unset
 
-    HttpHdrScTarget(const char *target_):
-        mask(0), max_age(MAX_AGE_UNSET), max_stale(MAX_STALE_UNSET),target(target_) {}
-    HttpHdrScTarget(const String &target_):
-        mask(0), max_age(MAX_AGE_UNSET), max_stale(MAX_STALE_UNSET),target(target_) {}
-    HttpHdrScTarget(const HttpHdrScTarget &t):
-        mask(t.mask), max_age(t.max_age), max_stale(t.max_stale),
-        content_(t.content_), target(t.target) {}
+    explicit HttpHdrScTarget(const char *aTarget): target(aTarget) {}
+    explicit HttpHdrScTarget(const String &aTarget): target(aTarget) {}
+    explicit HttpHdrScTarget(const HttpHdrScTarget &) = delete; // avoid accidental string copies
+    HttpHdrScTarget &operator =(const HttpHdrScTarget &) = delete; // avoid accidental string copies
 
     bool hasNoStore() const {return isSet(SC_NO_STORE); }
     void noStore(bool v) { setMask(SC_NO_STORE,v); }
@@ -93,12 +89,11 @@ private:
         else EBIT_CLR(mask,id);
     }
 
-    int mask;
-    int max_age;
-    int max_stale;
+    int mask = 0;
+    int max_age = MAX_AGE_UNSET;
+    int max_stale = MAX_STALE_UNSET;
     String content_;
     String target;
-    dlink_node node;
 };
 
 void httpHdrScTargetStatDumper(StoreEntry * sentry, int idx, double val, double size, int count);

diff --git a/src/http/forward.h b/src/http/forward.h
index ac51762d93a5d798ed52f749af1bfaadc2cbaf7f..6907ecf552b165ddf767d13b3cc2ba25b341a3cc 100644 (file)
--- a/src/http/forward.h
+++ b/src/http/forward.h
@@ -24,7 +24,19 @@ typedef RefCount<Http::Stream> StreamPointer;
 
 } // namespace Http
 
-// TODO move these classes into Http namespace
+// TODO move these into Http namespace
+
+typedef enum {
+    SC_NO_STORE,
+    SC_NO_STORE_REMOTE,
+    SC_MAX_AGE,
+    SC_CONTENT,
+    SC_OTHER,
+    SC_ENUM_END /* also used to mean "invalid" */
+} http_hdr_sc_type;
+
+class HttpHdrSc;
+
 class HttpRequestMethod;
 typedef RefCount<HttpRequestMethod> HttpRequestMethodPointer;