From: Amos Jeffries Date: Thu, 20 Aug 2020 12:40:55 +0000 (+0000) Subject: Remove dlink from HttpHdrSc (#589) X-Git-Tag: 4.15-20210522-snapshot~67 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=24673a9e1b5b991c78b0c6ae47920a6bd2b17bee;p=thirdparty%2Fsquid.git 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 a30498230d..9baf1a31b2 100644 --- 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(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(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(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(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 d0be883e28..d2dcf60261 100644 --- 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 + 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> targets; }; /* Http Surrogate Control Header Field */ diff --git a/src/HttpHdrScTarget.h b/src/HttpHdrScTarget.h index bb55ccadb8..b227009431 100644 --- 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 ac51762d93..6907ecf552 100644 --- a/src/http/forward.h +++ b/src/http/forward.h @@ -24,7 +24,19 @@ typedef RefCount 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 HttpRequestMethodPointer;