From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Sat, 23 Mar 2024 21:24:03 +0000 (+0000)
Subject: HTTP: Protect just-parsed responses from accidental destruction (#1752)
X-Git-Tag: SQUID_7_0_1~162
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=80a6a08a94a8c1029b907169e5803fe2e5e9a422;p=thirdparty%2Fsquid.git

HTTP: Protect just-parsed responses from accidental destruction (#1752)

The lack of HttpReply locking meant that we could not (safely) pass the
freshly created reply object to ACLFilledChecklist because the checklist
destructor would unwittingly destroy the object (by unlocking it). There
is at least one applicable ACL check in handle1xx(), but we got lucky
because that method adds the reply object to ALE, increasing its
reference counter. This change stops relying on such "external" locks.

This change also protects from HttpReply memory leaks when an exception
is thrown between HttpReply creation and a setVirginReply() call.

TODO: Fix all cases where newly created HttpReply objects are remembered
using raw pointers (but may be passed to locking/unlocking code).
---

diff --git a/src/http.cc b/src/http.cc
index 56223e3066..ae12b27f0e 100644
--- a/src/http.cc
+++ b/src/http.cc
@@ -702,7 +702,7 @@ HttpStateData::processReplyHeader()
     // reset payload tracking to begin after message headers
     payloadSeen = inBuf.length();
 
-    HttpReply *newrep = new HttpReply;
+    const auto newrep = HttpReply::Pointer::Make();
     // XXX: RFC 7230 indicates we MAY ignore the reason phrase,
     //      and use an empty string on unknown status.
     //      We do that now to avoid performance regression from using SBuf::c_str()
@@ -720,7 +720,7 @@ HttpStateData::processReplyHeader()
     newrep->sources |= request->url.getScheme() == AnyP::PROTO_HTTPS ? Http::Message::srcHttps : Http::Message::srcHttp;
 
     if (newrep->sline.version.protocol == AnyP::PROTO_HTTP && Http::Is1xx(newrep->sline.status())) {
-        handle1xx(newrep);
+        handle1xx(newrep.getRaw());
         return;
     }
 
@@ -733,7 +733,7 @@ HttpStateData::processReplyHeader()
     if (!peerSupportsConnectionPinning())
         request->flags.connectionAuthDisabled = true;
 
-    HttpReply *vrep = setVirginReply(newrep);
+    const auto vrep = setVirginReply(newrep.getRaw());
     flags.headers_parsed = true;
 
     keepaliveAccounting(vrep);
@@ -745,13 +745,11 @@ HttpStateData::processReplyHeader()
 
 /// ignore or start forwarding the 1xx response (a.k.a., control message)
 void
-HttpStateData::handle1xx(HttpReply *reply)
+HttpStateData::handle1xx(const HttpReply::Pointer &reply)
 {
     if (fwd->al)
         fwd->al->reply = reply;
 
-    HttpReply::Pointer msg(reply); // will destroy reply if unused
-
     // one 1xx at a time: we must not be called while waiting for previous 1xx
     Must(!flags.handling1xx);
     flags.handling1xx = true;
@@ -774,7 +772,7 @@ HttpStateData::handle1xx(HttpReply *reply)
     if (Config.accessList.reply) {
         ACLFilledChecklist ch(Config.accessList.reply, originalRequest().getRaw());
         ch.al = fwd->al;
-        ch.reply = reply;
+        ch.reply = reply.getRaw();
         ch.syncAle(originalRequest().getRaw(), nullptr);
         HTTPMSGLOCK(ch.reply);
         if (!ch.fastCheck().allowed()) // TODO: support slow lookups?
@@ -794,7 +792,7 @@ HttpStateData::handle1xx(HttpReply *reply)
     const AsyncCall::Pointer cb = JobCallback(11, 3, CbDialer, this,
                                   HttpStateData::proceedAfter1xx);
     CallJobHere1(11, 4, request->clientConnectionManager, ConnStateData,
-                 ConnStateData::sendControlMsg, HttpControlMsg(msg, cb));
+                 ConnStateData::sendControlMsg, HttpControlMsg(reply, cb));
     // If the call is not fired, then the Sink is gone, and HttpStateData
     // will terminate due to an aborted store entry or another similar error.
     // If we get stuck, it is not handle1xx fault if we could get stuck
diff --git a/src/http.h b/src/http.h
index cb4ae061d4..e10603123b 100644
--- a/src/http.h
+++ b/src/http.h
@@ -82,7 +82,7 @@ protected:
 
     void processReply();
     void proceedAfter1xx();
-    void handle1xx(HttpReply *msg);
+    void handle1xx(const HttpReplyPointer &);
     void drop1xx(const char *reason);
 
 private: