From: Alex Rousskov Date: Wed, 5 Jul 2023 14:41:47 +0000 (+0000) Subject: Do not leak memory when handling cache manager requests (#1408) X-Git-Tag: SQUID_7_0_1~406 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=182faab8059d973eed5d035daa61a08399badd20;p=thirdparty%2Fsquid.git Do not leak memory when handling cache manager requests (#1408) Also adjusted Cache-Control APIs to prevent similar bugs. These changes also speed up processing a bit and simplify most of the affected code. The now-gone "just remove the old CC" putCc() misfeature was unused. The leak was introduced by commit 92a5adb: PutCommonResponseHeaders() incorrectly assumed that putCc(pointerToX) takes ownership of X. Detected by Coverity. CID 1534779: Resource leak (RESOURCE_LEAK). --- diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc index b82f4431db..6d47d1bfee 100644 --- a/src/HttpHeader.cc +++ b/src/HttpHeader.cc @@ -1040,15 +1040,14 @@ HttpHeader::putAuth(const char *auth_scheme, const char *realm) } void -HttpHeader::putCc(const HttpHdrCc * cc) +HttpHeader::putCc(const HttpHdrCc &cc) { - assert(cc); /* remove old directives if any */ delById(Http::HdrType::CACHE_CONTROL); /* pack into mb */ MemBuf mb; mb.init(); - cc->packInto(&mb); + cc.packInto(&mb); /* put */ addEntry(new HttpHeaderEntry(Http::HdrType::CACHE_CONTROL, SBuf(), mb.buf)); /* cleanup */ diff --git a/src/HttpHeader.h b/src/HttpHeader.h index 698372c20e..0e529eca28 100644 --- a/src/HttpHeader.h +++ b/src/HttpHeader.h @@ -137,7 +137,7 @@ public: void putTime(Http::HdrType id, time_t htime); void putStr(Http::HdrType id, const char *str); void putAuth(const char *auth_scheme, const char *realm); - void putCc(const HttpHdrCc * cc); + void putCc(const HttpHdrCc &cc); void putContRange(const HttpHdrContRange * cr); void putRange(const HttpHdrRange * range); void putSc(HttpHdrSc *sc); diff --git a/src/HttpReply.cc b/src/HttpReply.cc index dbf9ceb1ff..4c19083cc0 100644 --- a/src/HttpReply.cc +++ b/src/HttpReply.cc @@ -148,7 +148,8 @@ HttpReply::make304() const rv->header.addEntry(e->clone()); } - rv->putCc(cache_control); + if (cache_control) + rv->putCc(*cache_control); /* rv->body */ return rv; diff --git a/src/cache_manager.cc b/src/cache_manager.cc index dfaefed5ca..828b95dde5 100644 --- a/src/cache_manager.cc +++ b/src/cache_manager.cc @@ -453,13 +453,13 @@ CacheManager::PutCommonResponseHeaders(HttpReply &response, const char *httpOrig response.header.putExt("Access-Control-Expose-Headers", "Server"); } - std::unique_ptr cc(new HttpHdrCc()); + HttpHdrCc cc; // this is honored by more caches but allows pointless revalidation; // revalidation will always fail because we do not support it (yet?) - cc->noCache(String()); + cc.noCache(String()); // this is honored by fewer caches but prohibits pointless revalidation - cc->noStore(true); - response.putCc(cc.release()); + cc.noStore(true); + response.putCc(cc); } CacheManager* diff --git a/src/ftp/Elements.cc b/src/ftp/Elements.cc index dc7d17efcb..766e465efd 100644 --- a/src/ftp/Elements.cc +++ b/src/ftp/Elements.cc @@ -40,7 +40,7 @@ Ftp::HttpReplyWrapper(const int ftpStatus, const char *ftpReason, const Http::St { HttpHdrCc cc; cc.Private(String()); - header.putCc(&cc); + header.putCc(cc); } if (ftpStatus > 0) header.putInt(Http::HdrType::FTP_STATUS, ftpStatus); diff --git a/src/http.cc b/src/http.cc index 70cef385ce..acdecb3e43 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1978,7 +1978,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request, if (flags.only_if_cached) cc->onlyIfCached(true); - hdr_out->putCc(cc); + hdr_out->putCc(*cc); delete cc; } diff --git a/src/http/Message.cc b/src/http/Message.cc index 88b8f3f7c3..17e73f4093 100644 --- a/src/http/Message.cc +++ b/src/http/Message.cc @@ -30,22 +30,11 @@ Http::Message::~Message() } void -Http::Message::putCc(const HttpHdrCc *otherCc) +Http::Message::putCc(const HttpHdrCc &otherCc) { - // get rid of the old CC, if any - if (cache_control) { - delete cache_control; - cache_control = nullptr; - if (!otherCc) - header.delById(Http::HdrType::CACHE_CONTROL); - // else it will be deleted inside putCc() below - } - - // add new CC, if any - if (otherCc) { - cache_control = new HttpHdrCc(*otherCc); - header.putCc(cache_control); - } + delete cache_control; + cache_control = new HttpHdrCc(otherCc); + header.putCc(*cache_control); } /* find first CRLF */ diff --git a/src/http/Message.h b/src/http/Message.h index b205d1c9ff..279f68d312 100644 --- a/src/http/Message.h +++ b/src/http/Message.h @@ -98,8 +98,9 @@ public: uint32_t sources = 0; ///< The message sources - /// copies Cache-Control header to this message - void putCc(const HttpHdrCc *otherCc); + /// copies Cache-Control header to this message, + /// overwriting existing Cache-Control header(s), if any + void putCc(const HttpHdrCc &); // returns true and sets hdr_sz on success // returns false and sets *error to zero when needs more data diff --git a/src/mime.cc b/src/mime.cc index 4d535f7f5e..452bf58857 100644 --- a/src/mime.cc +++ b/src/mime.cc @@ -421,7 +421,7 @@ MimeIcon::load() reply->setHeaders(status, nullptr, mimeGetContentType(icon_.c_str()), sb.st_size, sb.st_mtime, -1); reply->cache_control = new HttpHdrCc(); reply->cache_control->maxAge(86400); - reply->header.putCc(reply->cache_control); + reply->header.putCc(*reply->cache_control); e->replaceHttpReply(reply.getRaw()); if (status == Http::scOkay) { diff --git a/src/tests/stub_HttpHeader.cc b/src/tests/stub_HttpHeader.cc index f0eea0ab04..d6dc7f5e59 100644 --- a/src/tests/stub_HttpHeader.cc +++ b/src/tests/stub_HttpHeader.cc @@ -59,7 +59,7 @@ void HttpHeader::putInt64(Http::HdrType, int64_t ) STUB void HttpHeader::putTime(Http::HdrType, time_t) STUB void HttpHeader::putStr(Http::HdrType, const char *) STUB void HttpHeader::putAuth(const char *, const char *) STUB -void HttpHeader::putCc(const HttpHdrCc *) STUB +void HttpHeader::putCc(const HttpHdrCc &) STUB void HttpHeader::putContRange(const HttpHdrContRange *) STUB void HttpHeader::putRange(const HttpHdrRange *) STUB void HttpHeader::putSc(HttpHdrSc *) STUB diff --git a/src/tests/stub_libhttp.cc b/src/tests/stub_libhttp.cc index c8447dc76b..5460ce651a 100644 --- a/src/tests/stub_libhttp.cc +++ b/src/tests/stub_libhttp.cc @@ -42,7 +42,7 @@ Message::~Message() {STUB} void Message::packInto(Packable *, bool) const STUB void Message::setContentLength(int64_t) STUB bool Message::persistent() const STUB_RETVAL(false) -void Message::putCc(const HttpHdrCc *) STUB +void Message::putCc(const HttpHdrCc &) STUB bool Message::parse(const char *, const size_t, bool, Http::StatusCode *) STUB_RETVAL(false) bool Message::parseCharBuf(const char *, ssize_t) STUB_RETVAL(false) int Message::httpMsgParseStep(const char *, int, int) STUB_RETVAL(-1)