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 56223e3066577650abe2f0328d9f24aef33180b0..ae12b27f0eb5eb7eda406b07d1ad06fbe81c441e 100644 (file)
--- 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 cb4ae061d4329371ab2139df8da55180ca2d0a3d..e10603123b74c7987294aa244a035a3733e03872 100644 (file)
--- 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: