From: Eduard Bagdasaryan Date: Fri, 5 May 2017 19:23:07 +0000 (-0600) Subject: Reduced the chance of using an absent HttpHdrCc directive value. X-Git-Tag: M-staged-PR71~184 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=810d879f8beedc3ca6c59ef95e7cc2acf4249abd;p=thirdparty%2Fsquid.git Reduced the chance of using an absent HttpHdrCc directive value. Also reduced code duplication inside HttpReply::hdrExpirationTime(). --- diff --git a/src/HttpHdrCc.cc b/src/HttpHdrCc.cc index 34ddd4628f..cd07978298 100644 --- a/src/HttpHdrCc.cc +++ b/src/HttpHdrCc.cc @@ -264,13 +264,13 @@ HttpHdrCc::packInto(Packable * p) const case HttpHdrCcType::CC_PUBLIC: break; case HttpHdrCcType::CC_PRIVATE: - if (Private().size()) - p->appendf("=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(Private())); + if (private_.size()) + p->appendf("=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(private_)); break; case HttpHdrCcType::CC_NO_CACHE: - if (noCache().size()) - p->appendf("=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(noCache())); + if (no_cache.size()) + p->appendf("=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(no_cache)); break; case HttpHdrCcType::CC_NO_STORE: break; @@ -281,24 +281,24 @@ HttpHdrCc::packInto(Packable * p) const case HttpHdrCcType::CC_PROXY_REVALIDATE: break; case HttpHdrCcType::CC_MAX_AGE: - p->appendf("=%d", maxAge()); + p->appendf("=%d", max_age); break; case HttpHdrCcType::CC_S_MAXAGE: - p->appendf("=%d", sMaxAge()); + p->appendf("=%d", s_maxage); break; case HttpHdrCcType::CC_MAX_STALE: /* max-stale's value is optional. If we didn't receive it, don't send it */ - if (maxStale()!=MAX_STALE_ANY) - p->appendf("=%d", maxStale()); + if (max_stale != MAX_STALE_ANY) + p->appendf("=%d", max_stale); break; case HttpHdrCcType::CC_MIN_FRESH: - p->appendf("=%d", minFresh()); + p->appendf("=%d", min_fresh); break; case HttpHdrCcType::CC_ONLY_IF_CACHED: break; case HttpHdrCcType::CC_STALE_IF_ERROR: - p->appendf("=%d", staleIfError()); + p->appendf("=%d", stale_if_error); break; case HttpHdrCcType::CC_IMMUTABLE: break; diff --git a/src/HttpHdrCc.h b/src/HttpHdrCc.h index 6672e59063..2aa300f716 100644 --- a/src/HttpHdrCc.h +++ b/src/HttpHdrCc.h @@ -67,13 +67,11 @@ public: //manipulation for Cache-Control: public header bool hasPublic() const {return isSet(HttpHdrCcType::CC_PUBLIC);} - bool Public() const {return isSet(HttpHdrCcType::CC_PUBLIC);} void Public(bool v) {setMask(HttpHdrCcType::CC_PUBLIC,v);} void clearPublic() {setMask(HttpHdrCcType::CC_PUBLIC,false);} //manipulation for Cache-Control: private header - bool hasPrivate() const {return isSet(HttpHdrCcType::CC_PRIVATE);} - const String &Private() const {return private_;} + bool hasPrivate(const String **val = nullptr) const { return hasDirective(HttpHdrCcType::CC_PRIVATE, &private_, val); } void Private(const String &v) { setMask(HttpHdrCcType::CC_PRIVATE,true); if (!v.size()) @@ -86,8 +84,9 @@ public: void clearPrivate() {setMask(HttpHdrCcType::CC_PRIVATE,false); private_.clean();} //manipulation for Cache-Control: no-cache header - bool hasNoCache() const {return isSet(HttpHdrCcType::CC_NO_CACHE);} - const String &noCache() const {return no_cache;} + bool hasNoCacheWithParameters() const { return hasNoCache() && no_cache.size(); } + bool hasNoCacheWithoutParameters() const { return hasNoCache() && !no_cache.size(); } + bool hasNoCache(const String **val = nullptr) const { return hasDirective(HttpHdrCcType::CC_NO_CACHE, &no_cache, val); } void noCache(const String &v) { setMask(HttpHdrCcType::CC_NO_CACHE,true); if (!v.size()) @@ -101,43 +100,36 @@ public: //manipulation for Cache-Control: no-store header bool hasNoStore() const {return isSet(HttpHdrCcType::CC_NO_STORE);} - bool noStore() const {return isSet(HttpHdrCcType::CC_NO_STORE);} void noStore(bool v) {setMask(HttpHdrCcType::CC_NO_STORE,v);} void clearNoStore() {setMask(HttpHdrCcType::CC_NO_STORE,false);} //manipulation for Cache-Control: no-transform header bool hasNoTransform() const {return isSet(HttpHdrCcType::CC_NO_TRANSFORM);} - bool noTransform() const {return isSet(HttpHdrCcType::CC_NO_TRANSFORM);} void noTransform(bool v) {setMask(HttpHdrCcType::CC_NO_TRANSFORM,v);} void clearNoTransform() {setMask(HttpHdrCcType::CC_NO_TRANSFORM,false);} //manipulation for Cache-Control: must-revalidate header bool hasMustRevalidate() const {return isSet(HttpHdrCcType::CC_MUST_REVALIDATE);} - bool mustRevalidate() const {return isSet(HttpHdrCcType::CC_MUST_REVALIDATE);} void mustRevalidate(bool v) {setMask(HttpHdrCcType::CC_MUST_REVALIDATE,v);} void clearMustRevalidate() {setMask(HttpHdrCcType::CC_MUST_REVALIDATE,false);} //manipulation for Cache-Control: proxy-revalidate header bool hasProxyRevalidate() const {return isSet(HttpHdrCcType::CC_PROXY_REVALIDATE);} - bool proxyRevalidate() const {return isSet(HttpHdrCcType::CC_PROXY_REVALIDATE);} void proxyRevalidate(bool v) {setMask(HttpHdrCcType::CC_PROXY_REVALIDATE,v);} void clearProxyRevalidate() {setMask(HttpHdrCcType::CC_PROXY_REVALIDATE,false);} //manipulation for Cache-Control: max-age header - bool hasMaxAge() const {return isSet(HttpHdrCcType::CC_MAX_AGE);} - int32_t maxAge() const { return max_age;} + bool hasMaxAge(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_MAX_AGE, max_age, val); } void maxAge(int32_t v) {setValue(max_age,v,HttpHdrCcType::CC_MAX_AGE); } void clearMaxAge() {setValue(max_age,MAX_AGE_UNKNOWN,HttpHdrCcType::CC_MAX_AGE,false);} //manipulation for Cache-Control: s-maxage header - bool hasSMaxAge() const {return isSet(HttpHdrCcType::CC_S_MAXAGE);} - int32_t sMaxAge() const { return s_maxage;} + bool hasSMaxAge(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_S_MAXAGE, s_maxage, val); } void sMaxAge(int32_t v) {setValue(s_maxage,v,HttpHdrCcType::CC_S_MAXAGE); } void clearSMaxAge() {setValue(s_maxage,MAX_AGE_UNKNOWN,HttpHdrCcType::CC_S_MAXAGE,false);} //manipulation for Cache-Control: max-stale header - bool hasMaxStale() const {return isSet(HttpHdrCcType::CC_MAX_STALE);} - int32_t maxStale() const { return max_stale;} + bool hasMaxStale(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_MAX_STALE, max_stale, val); } // max-stale has a special value (MAX_STALE_ANY) which correspond to having // the directive without a numeric specification, and directs to consider the object // as always-expired. @@ -145,25 +137,22 @@ public: void clearMaxStale() {setValue(max_stale,MAX_STALE_UNKNOWN,HttpHdrCcType::CC_MAX_STALE,false);} //manipulation for Cache-Control:min-fresh header - bool hasMinFresh() const {return isSet(HttpHdrCcType::CC_MIN_FRESH);} - int32_t minFresh() const { return min_fresh;} + bool hasMinFresh(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_MIN_FRESH, max_stale, val); } void minFresh(int32_t v) {if (v < 0) return; setValue(min_fresh,v,HttpHdrCcType::CC_MIN_FRESH); } void clearMinFresh() {setValue(min_fresh,MIN_FRESH_UNKNOWN,HttpHdrCcType::CC_MIN_FRESH,false);} //manipulation for Cache-Control: only-if-cached header bool hasOnlyIfCached() const {return isSet(HttpHdrCcType::CC_ONLY_IF_CACHED);} - bool onlyIfCached() const {return isSet(HttpHdrCcType::CC_ONLY_IF_CACHED);} void onlyIfCached(bool v) {setMask(HttpHdrCcType::CC_ONLY_IF_CACHED,v);} void clearOnlyIfCached() {setMask(HttpHdrCcType::CC_ONLY_IF_CACHED,false);} //manipulation for Cache-Control: stale-if-error header - bool hasStaleIfError() const {return isSet(HttpHdrCcType::CC_STALE_IF_ERROR);} - int32_t staleIfError() const { return stale_if_error;} + bool hasStaleIfError(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_STALE_IF_ERROR, stale_if_error, val); } void staleIfError(int32_t v) {setValue(stale_if_error,v,HttpHdrCcType::CC_STALE_IF_ERROR); } void clearStaleIfError() {setValue(stale_if_error,STALE_IF_ERROR_UNKNOWN,HttpHdrCcType::CC_STALE_IF_ERROR,false);} //manipulation for Cache-Control: immutable header - bool Immutable() const {return isSet(HttpHdrCcType::CC_IMMUTABLE);} + bool hasImmutable() const {return isSet(HttpHdrCcType::CC_IMMUTABLE);} void Immutable(bool v) {setMask(HttpHdrCcType::CC_IMMUTABLE,v);} void clearImmutable() {setMask(HttpHdrCcType::CC_IMMUTABLE,false);} @@ -190,6 +179,17 @@ private: String private_; ///< List of headers sent as value for CC:private="...". May be empty/undefined if the value is missing. String no_cache; ///< List of headers sent as value for CC:no-cache="...". May be empty/undefined if the value is missing. + /// implements typical has*() method logic + template + bool hasDirective(const HttpHdrCcType hdrType, const Value &parsedVal, Value *outVal = nullptr) const { + if (isSet(hdrType)) { + if (outVal) + *outVal = parsedVal; + return true; + } + return false; + } + /// low-level part of the public set method, performs no checks void setMask(HttpHdrCcType id, bool newval=true) { if (newval) diff --git a/src/HttpReply.cc b/src/HttpReply.cc index a0a0abedae..c715d0708f 100644 --- a/src/HttpReply.cc +++ b/src/HttpReply.cc @@ -262,24 +262,13 @@ HttpReply::hdrExpirationTime() /* The s-maxage and max-age directive takes priority over Expires */ if (cache_control) { - if (date >= 0) { - if (cache_control->hasSMaxAge()) - return date + cache_control->sMaxAge(); - - if (cache_control->hasMaxAge()) - return date + cache_control->maxAge(); - } else { - /* - * Conservatively handle the case when we have a max-age - * header, but no Date for reference? - */ - - if (cache_control->hasSMaxAge()) - return squid_curtime; - - if (cache_control->hasMaxAge()) - return squid_curtime; - } + int maxAge = -1; + /* + * Conservatively handle the case when we have a max-age + * header, but no Date for reference? + */ + if (cache_control->hasSMaxAge(&maxAge) || cache_control->hasMaxAge(&maxAge)) + return (date >= 0) ? date + maxAge : squid_curtime; } if (Config.onoff.vary_ignore_expire && diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 9cca8cbe39..1d21ff3fd5 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -548,7 +548,7 @@ HttpRequest::maybeCacheable() // // NP: refresh_pattern ignore-no-store only applies to response messages // this test is handling request message CC header. - if (!flags.ignoreCc && cache_control && cache_control->noStore()) + if (!flags.ignoreCc && cache_control && cache_control->hasNoStore()) return false; break; diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 27a7996adb..b955bd5a90 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -193,7 +193,7 @@ ClientHttpRequest::onlyIfCached()const { assert(request); return request->cache_control && - request->cache_control->onlyIfCached(); + request->cache_control->hasOnlyIfCached(); } /** diff --git a/src/http.cc b/src/http.cc index 3d7fb5c814..c193a5de89 100644 --- a/src/http.cc +++ b/src/http.cc @@ -284,7 +284,7 @@ HttpStateData::processSurrogateControl(HttpReply *reply) HttpHdrScTarget *sctusable = reply->surrogate_control->getMergedTarget(Config.Accel.surrogate_id); if (sctusable) { - if (sctusable->noStore() || + if (sctusable->hasNoStore() || (Config.onoff.surrogate_is_remote && sctusable->noStoreRemote())) { surrogateNoStore = true; @@ -361,14 +361,14 @@ HttpStateData::cacheableReply() // for now we are not reliably doing that so we waste CPU re-checking request CC // RFC 2616 section 14.9.2 - MUST NOT cache any response with request CC:no-store - if (request && request->cache_control && request->cache_control->noStore() && + if (request && request->cache_control && request->cache_control->hasNoStore() && !REFRESH_OVERRIDE(ignore_no_store)) { debugs(22, 3, HERE << "NO because client request Cache-Control:no-store"); return 0; } // NP: request CC:no-cache only means cache READ is forbidden. STORE is permitted. - if (rep->cache_control && rep->cache_control->hasNoCache() && rep->cache_control->noCache().size() > 0) { + if (rep->cache_control && rep->cache_control->hasNoCacheWithParameters()) { /* TODO: we are allowed to cache when no-cache= has parameters. * Provided we strip away any of the listed headers unless they are revalidated * successfully (ie, must revalidate AND these headers are prohibited on stale replies). @@ -382,7 +382,7 @@ HttpStateData::cacheableReply() // NP: other request CC flags are limiters on HIT/MISS. We don't care about here. // RFC 2616 section 14.9.2 - MUST NOT cache any response with CC:no-store - if (rep->cache_control && rep->cache_control->noStore() && + if (rep->cache_control && rep->cache_control->hasNoStore() && !REFRESH_OVERRIDE(ignore_no_store)) { debugs(22, 3, HERE << "NO because server reply Cache-Control:no-store"); return 0; @@ -419,12 +419,12 @@ HttpStateData::cacheableReply() bool mayStore = false; // HTTPbis pt6 section 3.2: a response CC:public is present - if (rep->cache_control->Public()) { + if (rep->cache_control->hasPublic()) { debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:public"); mayStore = true; // HTTPbis pt6 section 3.2: a response CC:must-revalidate is present - } else if (rep->cache_control->mustRevalidate()) { + } else if (rep->cache_control->hasMustRevalidate()) { debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:must-revalidate"); mayStore = true; @@ -433,13 +433,13 @@ HttpStateData::cacheableReply() // HTTPbis WG verdict on this is that it is omitted from the spec due to being 'unexpected' by // some. The caching+revalidate is not exactly unsafe though with Squids interpretation of no-cache // (without parameters) as equivalent to must-revalidate in the reply. - } else if (rep->cache_control->hasNoCache() && rep->cache_control->noCache().size() == 0) { + } else if (rep->cache_control->hasNoCacheWithoutParameters()) { debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:no-cache (equivalent to must-revalidate)"); mayStore = true; #endif // HTTPbis pt6 section 3.2: a response CC:s-maxage is present - } else if (rep->cache_control->sMaxAge()) { + } else if (rep->cache_control->hasSMaxAge()) { debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:s-maxage"); mayStore = true; } @@ -997,10 +997,10 @@ HttpStateData::haveParsedReplyHeaders() // For security reasons we do so even if storage was caused by refresh_pattern ignore-* option // CC:must-revalidate or CC:proxy-revalidate - const bool ccMustRevalidate = (rep->cache_control->proxyRevalidate() || rep->cache_control->mustRevalidate()); + const bool ccMustRevalidate = (rep->cache_control->hasProxyRevalidate() || rep->cache_control->hasMustRevalidate()); // CC:no-cache (only if there are no parameters) - const bool ccNoCacheNoParams = (rep->cache_control->hasNoCache() && rep->cache_control->noCache().size()==0); + const bool ccNoCacheNoParams = rep->cache_control->hasNoCacheWithoutParameters(); // CC:s-maxage=N const bool ccSMaxAge = rep->cache_control->hasSMaxAge(); diff --git a/src/refresh.cc b/src/refresh.cc index 9a72ad600b..dceb824e1c 100644 --- a/src/refresh.cc +++ b/src/refresh.cc @@ -309,8 +309,8 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta) if (request && !request->flags.ignoreCc) { const HttpHdrCc *const cc = request->cache_control; - if (cc && cc->hasMinFresh()) { - const int32_t minFresh=cc->minFresh(); + int minFresh = -1; + if (cc && cc->hasMinFresh(&minFresh)) { debugs(22, 3, "\tage + min-fresh:\t" << age << " + " << minFresh << " = " << age + minFresh); debugs(22, 3, "\tcheck_time + min-fresh:\t" << check_time << " + " @@ -330,9 +330,10 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta) const HttpReplyPointer reply(entry->mem_obj && entry->mem_obj->getReply() ? entry->mem_obj->getReply() : nullptr); // stale-if-error requires any failure be passed thru when its period is over. + int staleIfError = -1; if (request && reply && reply->cache_control && - reply->cache_control->hasStaleIfError() && - reply->cache_control->staleIfError() < staleness) { + reply->cache_control->hasStaleIfError(&staleIfError) && + staleIfError < staleness) { debugs(22, 3, "stale-if-error period expired. Will produce error if validation fails."); request->flags.failOnValidationError = true; @@ -416,32 +417,34 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta) if (NULL != cc) { // max-age directive - if (cc->hasMaxAge()) { + int maxAge = -1; + if (cc->hasMaxAge(&maxAge)) { // draft-mcmanus-immutable-00: reply contains CC:immutable then ignore client CC:max-age=N - if (reply && reply->cache_control && reply->cache_control->Immutable()) { - debugs(22, 3, "MAYBE: Ignoring client CC:max-age=" << cc->maxAge() << " request - 'Cache-Control: immutable'"); + if (reply && reply->cache_control && reply->cache_control->hasImmutable()) { + debugs(22, 3, "MAYBE: Ignoring client CC:max-age=" << maxAge << " request - 'Cache-Control: immutable'"); #if USE_HTTP_VIOLATIONS // Ignore of client "Cache-Control: max-age=0" header - } else if (R->flags.ignore_reload && cc->maxAge() == 0) { + } else if (R->flags.ignore_reload && maxAge == 0) { debugs(22, 3, "MAYBE: Ignoring client reload request - trying to serve from cache (ignore-reload option)"); #endif // Honour client "Cache-Control: max-age=x" header - } else if (age > cc->maxAge() || cc->maxAge() == 0) { - debugs(22, 3, "YES: Revalidating object - client 'Cache-Control: max-age=" << cc->maxAge() << "'"); + } else if (age > maxAge || maxAge == 0) { + debugs(22, 3, "YES: Revalidating object - client 'Cache-Control: max-age=" << maxAge << "'"); return STALE_EXCEEDS_REQUEST_MAX_AGE_VALUE; } } // max-stale directive - if (cc->hasMaxStale() && staleness > -1) { - if (cc->maxStale()==HttpHdrCc::MAX_STALE_ANY) { + int maxStale = -1; + if (cc->hasMaxStale(&maxStale) && staleness > -1) { + if (maxStale==HttpHdrCc::MAX_STALE_ANY) { debugs(22, 3, "NO: Client accepts a stale response of any age - 'Cache-Control: max-stale'"); return FRESH_REQUEST_MAX_STALE_ALL; - } else if (staleness < cc->maxStale()) { - debugs(22, 3, "NO: Client accepts a stale response - 'Cache-Control: max-stale=" << cc->maxStale() << "'"); + } else if (staleness < maxStale) { + debugs(22, 3, "NO: Client accepts a stale response - 'Cache-Control: max-stale=" << maxStale << "'"); return FRESH_REQUEST_MAX_STALE_VALUE; } }