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 b82f4431db5078408715c793b7f6925e022340d6..6d47d1bfee727c3e53760ee0b98f0efb300dfdc2 100644 (file)
--- 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 698372c20e4174be05ef5ebfdba535453355acd5..0e529eca288c478c9b49563f3c2b681ad75f3b20 100644 (file)
--- 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 dbf9ceb1ff4cbd5c29a9d70e904cdc11ebb58338..4c19083cc043a2307b680ff02b6836f278edde4a 100644 (file)
--- 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 dfaefed5ca57cc8e88e1e47415fbceaa84e9d576..828b95dde52fd832203b35fd1b6928c18ad0384d 100644 (file)
--- 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<HttpHdrCc> 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 dc7d17efcb8266e8b3bc90142548836149bbb05c..766e465efd6fed67a195aa8305d47546dc06fd46 100644 (file)
--- 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 70cef385ce77b50f81634a0529dabd6113e50ad1..acdecb3e43bf394fbf4cab0bbb174108dcb7f1bd 100644 (file)
--- 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 88b8f3f7c3ea16191e3af948e6eda842a985ac22..17e73f409332163f59009995f918dacd25759d6e 100644 (file)
--- 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 b205d1c9ff3dc84e4913f5895b9361127636eba9..279f68d31298d1f48d27e676dde24df67cb138c7 100644 (file)
--- 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 4d535f7f5e13b2d225c6c73d11d6ce66e5f185be..452bf5885716fd9f21057bc4f6ef34b8ddd18c59 100644 (file)
--- 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 f0eea0ab0435c9e2d333fe36ea9a56273610ca42..d6dc7f5e59dcb5a936fee536b6c80d1d7953b6aa 100644 (file)
--- 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 c8447dc76b2012570bc23793d49766edc8d22ca5..5460ce651a4cf8c2617c89bfcccf29a2cf7bd54f 100644 (file)
--- 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)