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 7875793f074fb9d6cab10a0d750aee5afe80d70b..c7c7288ef1f5ced05d89a29e5d0530d33e606f4e 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 9c14985f90a03de9f39fa3a01005eb51861bdde2..47597be4bb18f71db6be00136e78a8501c4b3921 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 9348fdc1287ebafbda4ad859072a413a2bdf834c..b09b258f475d61625792c00bb6dd3a6b732da6a7 100644 (file)
--- a/src/cache_manager.cc
+++ b/src/cache_manager.cc
@@ -484,13 +484,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 0829c25142fc801d275278cf7a5f253d183592bd..4916c1520af3190f0f8471d1c9d1ed292dd4dcc4 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -2032,7 +2032,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 6e4f879ed71135119ddbab603598149dddc36e45..19908fd725db8718446592c7c9ab8998cd5cf93e 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 697d3beb7330eeeb93e198f0a8e76479cd1b622c..7536e9e27da7a642601c53d8c5969a7ab7648403 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)