Bug 1528481 - Possible use after free in Http2Session::RecvPushPromise() if we have a usable entry in the cache for the resource. r=dragana, a=ritu
authorMichal Novotny <michal.novotny@gmail.com>
Fri, 14 Jun 2019 22:54:06 +0000
changeset 537065 3d562db5d1ed358a18da5bbe160c1da4864ef6c0
parent 537064 4174d76d91945a7261f81d7666eee4b2b35cb027
child 537066 60e8af0747b4ae12b440b3e406ffedd4ccc71d5f
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdragana, ritu
bugs1528481
milestone68.0
Bug 1528481 - Possible use after free in Http2Session::RecvPushPromise() if we have a usable entry in the cache for the resource. r=dragana, a=ritu If we find a usable cache entry, the stream is closed and release in CachePushCheckCallback::OnCacheEntryCheck(). This patch converts raw pointer to a weak pointer, so we can simply check if the object still exists after AsyncOpenURI() finishes. Differential Revision: https://phabricator.services.mozilla.com/D33945
netwerk/protocol/http/Http2Session.cpp
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -1911,81 +1911,82 @@ nsresult Http2Session::RecvPushPromise(H
     Telemetry::Accumulate(Telemetry::SPDY_CONTINUED_HEADERS,
                           self->mAggregatedHeaderSize);
   }
 
   // Create the buffering transaction and push stream
   RefPtr<Http2PushTransactionBuffer> transactionBuffer =
       new Http2PushTransactionBuffer();
   transactionBuffer->SetConnection(self);
-  Http2PushedStream* pushedStream = new Http2PushedStream(
+  nsAutoPtr<Http2PushedStream> pushedStream(new Http2PushedStream(
       transactionBuffer, self, associatedStream, promisedID,
-      self->mCurrentForegroundTabOuterContentWindowId);
+      self->mCurrentForegroundTabOuterContentWindowId));
 
   rv = pushedStream->ConvertPushHeaders(&self->mDecompressor,
                                         self->mDecompressBuffer,
                                         pushedStream->GetRequestString());
 
   if (rv == NS_ERROR_NOT_IMPLEMENTED) {
     LOG3(("Http2Session::PushPromise Semantics not Implemented\n"));
     self->GenerateRstStream(REFUSED_STREAM_ERROR, promisedID);
-    delete pushedStream;
     self->ResetDownstreamState();
     return NS_OK;
   }
 
   if (rv == NS_ERROR_ILLEGAL_VALUE) {
     // This means the decompression completed ok, but there was a problem with
     // the decoded headers. Reset the stream and go away.
     self->GenerateRstStream(PROTOCOL_ERROR, promisedID);
-    delete pushedStream;
     self->ResetDownstreamState();
     return NS_OK;
   } else if (NS_FAILED(rv)) {
     // This is fatal to the session.
     self->mGoAwayReason = COMPRESSION_ERROR;
     return rv;
   }
 
+  WeakPtr<Http2Stream> pushedWeak = pushedStream.forget();
+
   // Ownership of the pushed stream is by the transaction hash, just as it
   // is for a client initiated stream. Errors that aren't fatal to the
   // whole session must call cleanupStream() after this point in order
   // to remove the stream from that hash.
-  self->mStreamTransactionHash.Put(transactionBuffer, pushedStream);
-  self->mPushedStreams.AppendElement(pushedStream);
-
-  if (self->RegisterStreamID(pushedStream, promisedID) == kDeadStreamID) {
+  self->mStreamTransactionHash.Put(transactionBuffer, pushedWeak);
+  self->mPushedStreams.AppendElement(
+      static_cast<Http2PushedStream*>(pushedWeak.get()));
+
+  if (self->RegisterStreamID(pushedWeak, promisedID) == kDeadStreamID) {
     LOG3(("Http2Session::RecvPushPromise registerstreamid failed\n"));
     self->mGoAwayReason = INTERNAL_ERROR;
     return NS_ERROR_FAILURE;
   }
 
   if (promisedID > self->mOutgoingGoAwayID)
     self->mOutgoingGoAwayID = promisedID;
 
   // Fake the request side of the pushed HTTP transaction. Sets up hash
   // key and origin
   uint32_t notUsed;
-  Unused << pushedStream->ReadSegments(nullptr, 1, &notUsed);
+  Unused << pushedWeak->ReadSegments(nullptr, 1, &notUsed);
 
   nsAutoCString key;
-  if (!pushedStream->GetHashKey(key)) {
+  if (!static_cast<Http2PushedStream*>(pushedWeak.get())->GetHashKey(key)) {
     LOG3(
         ("Http2Session::RecvPushPromise one of :authority :scheme :path "
          "missing from push\n"));
-    self->CleanupStream(pushedStream, NS_ERROR_FAILURE, PROTOCOL_ERROR);
+    self->CleanupStream(pushedWeak, NS_ERROR_FAILURE, PROTOCOL_ERROR);
     self->ResetDownstreamState();
     return NS_OK;
   }
 
   // does the pushed origin belong on this connection?
   LOG3(("Http2Session::RecvPushPromise %p origin check %s", self,
-        pushedStream->Origin().get()));
+        pushedWeak->Origin().get()));
   nsCOMPtr<nsIURI> pushedOrigin;
-  rv = Http2Stream::MakeOriginURL(pushedStream->Origin(), pushedOrigin);
+  rv = Http2Stream::MakeOriginURL(pushedWeak->Origin(), pushedOrigin);
   nsAutoCString pushedHostName;
   int32_t pushedPort = -1;
   if (NS_SUCCEEDED(rv)) {
     rv = pushedOrigin->GetHost(pushedHostName);
   }
   if (NS_SUCCEEDED(rv)) {
     rv = pushedOrigin->GetPort(&pushedPort);
     if (NS_SUCCEEDED(rv) && pushedPort == -1) {
@@ -1997,83 +1998,90 @@ nsresult Http2Session::RecvPushPromise(H
       } else {
         pushedPort = NS_HTTPS_DEFAULT_PORT;
       }
     }
   }
   if (NS_FAILED(rv) || !self->TestJoinConnection(pushedHostName, pushedPort)) {
     LOG3((
         "Http2Session::RecvPushPromise %p pushed stream mismatched origin %s\n",
-        self, pushedStream->Origin().get()));
-    self->CleanupStream(pushedStream, NS_ERROR_FAILURE, REFUSED_STREAM_ERROR);
+        self, pushedWeak->Origin().get()));
+    self->CleanupStream(pushedWeak, NS_ERROR_FAILURE, REFUSED_STREAM_ERROR);
     self->ResetDownstreamState();
     return NS_OK;
   }
 
-  if (pushedStream->TryOnPush()) {
+  if (static_cast<Http2PushedStream*>(pushedWeak.get())->TryOnPush()) {
     LOG3(
         ("Http2Session::RecvPushPromise %p channel implements "
          "nsIHttpPushListener "
          "stream %p will not be placed into session cache.\n",
-         self, pushedStream));
+         self, pushedWeak.get()));
   } else {
     LOG3(("Http2Session::RecvPushPromise %p place stream into session cache\n",
           self));
-    if (!cache->RegisterPushedStreamHttp2(key, pushedStream)) {
+    if (!cache->RegisterPushedStreamHttp2(
+            key, static_cast<Http2PushedStream*>(pushedWeak.get()))) {
       // This only happens if they've already pushed us this item.
       LOG3(("Http2Session::RecvPushPromise registerPushedStream Failed\n"));
-      self->CleanupStream(pushedStream, NS_ERROR_FAILURE, REFUSED_STREAM_ERROR);
+      self->CleanupStream(pushedWeak, NS_ERROR_FAILURE, REFUSED_STREAM_ERROR);
       self->ResetDownstreamState();
       return NS_OK;
     }
 
     // Kick off a lookup into the HTTP cache so we can cancel the push if it's
     // unneeded (we already have it in our local regular cache). See bug
     // 1367551.
     nsCOMPtr<nsICacheStorageService> css =
         do_GetService("@mozilla.org/netwerk/cache-storage-service;1");
     mozilla::OriginAttributes oa;
-    pushedStream->GetOriginAttributes(&oa);
+    pushedWeak->GetOriginAttributes(&oa);
     RefPtr<LoadContextInfo> lci = GetLoadContextInfo(false, oa);
     nsCOMPtr<nsICacheStorage> ds;
     css->DiskCacheStorage(lci, false, getter_AddRefs(ds));
     // Build up our full URL for the cache lookup
     nsAutoCString spec;
-    spec.Assign(pushedStream->Origin());
-    spec.Append(pushedStream->Path());
+    spec.Assign(pushedWeak->Origin());
+    spec.Append(pushedWeak->Path());
     nsCOMPtr<nsIURI> pushedURL;
     // Nifty trick: this doesn't actually do anything origin-specific, it's just
     // named that way. So by passing it the full spec here, we get a URL with
     // the full path.
     // Another nifty trick! Even though this is using nsIURIs (which are not
     // generally ok off the main thread), since we're not using the protocol
     // handler to create any URIs, this will work just fine here. Don't try this
     // at home, though, kids. I'm a trained professional.
     if (NS_SUCCEEDED(Http2Stream::MakeOriginURL(spec, pushedURL))) {
       LOG3(("Http2Session::RecvPushPromise %p check disk cache for entry",
             self));
       RefPtr<CachePushCheckCallback> cpcc = new CachePushCheckCallback(
-          self, promisedID, pushedStream->GetRequestString());
+          self, promisedID,
+          static_cast<Http2PushedStream*>(pushedWeak.get())
+              ->GetRequestString());
       if (NS_FAILED(ds->AsyncOpenURI(
               pushedURL, EmptyCString(),
               nsICacheStorage::OPEN_READONLY | nsICacheStorage::OPEN_SECRETLY,
               cpcc))) {
         LOG3(
             ("Http2Session::RecvPushPromise %p failed to open cache entry for "
              "push check",
              self));
+      } else if (!pushedWeak) {
+        // We have an up to date entry in our cache, so the stream was closed
+        // and released in CachePushCheckCallback::OnCacheEntryCheck().
+        return NS_OK;
       }
     }
   }
 
-  pushedStream->SetHTTPState(Http2Stream::RESERVED_BY_REMOTE);
+  pushedWeak->SetHTTPState(Http2Stream::RESERVED_BY_REMOTE);
   static_assert(Http2Stream::kWorstPriority >= 0,
                 "kWorstPriority out of range");
-  uint32_t priorityDependency = pushedStream->PriorityDependency();
-  uint8_t priorityWeight = pushedStream->PriorityWeight();
+  uint32_t priorityDependency = pushedWeak->PriorityDependency();
+  uint8_t priorityWeight = pushedWeak->PriorityWeight();
   self->SendPriorityFrame(promisedID, priorityDependency, priorityWeight);
   self->ResetDownstreamState();
   return NS_OK;
 }
 
 NS_IMPL_ISUPPORTS(Http2Session::CachePushCheckCallback,
                   nsICacheEntryOpenCallback);