From 91ecc85e93cb693385f21ca372633e3964d5f90b Mon Sep 17 00:00:00 2001
From: Marshall Greenblatt <magreenblatt@gmail.com>
Date: Tue, 23 Mar 2021 15:58:17 -0400
Subject: [PATCH] Fix potential use-after-free of V8TrackArrayBuffer (fixes
 issue #3074)

---
 libcef/renderer/v8_impl.cc    | 35 ++++++++++++-----------------------
 tests/ceftests/v8_unittest.cc | 28 ++++++----------------------
 2 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/libcef/renderer/v8_impl.cc b/libcef/renderer/v8_impl.cc
index fd879fa79..30c998250 100644
--- a/libcef/renderer/v8_impl.cc
+++ b/libcef/renderer/v8_impl.cc
@@ -318,18 +318,14 @@ class V8TrackArrayBuffer : public CefTrackNode {
  public:
   explicit V8TrackArrayBuffer(
       v8::Isolate* isolate,
-      void* buffer,
       CefRefPtr<CefV8ArrayBufferReleaseCallback> release_callback)
-      : isolate_(isolate),
-        buffer_(buffer),
-        release_callback_(release_callback) {
+      : isolate_(isolate), release_callback_(release_callback) {
     DCHECK(isolate_);
     isolate_->AdjustAmountOfExternalAllocatedMemory(
         static_cast<int>(sizeof(V8TrackArrayBuffer)));
   }
 
   ~V8TrackArrayBuffer() {
-    ReleaseBuffer();
     isolate_->AdjustAmountOfExternalAllocatedMemory(
         -static_cast<int>(sizeof(V8TrackArrayBuffer)));
   }
@@ -338,15 +334,6 @@ class V8TrackArrayBuffer : public CefTrackNode {
     return release_callback_;
   }
 
-  void ReleaseBuffer() {
-    if (buffer_ && release_callback_) {
-      release_callback_->ReleaseBuffer(buffer_);
-    }
-    Detach();
-  }
-
-  void Detach() { buffer_ = nullptr; }
-
   // Attach this track object to the specified V8 object.
   void AttachTo(v8::Local<v8::Context> context,
                 v8::Local<v8::ArrayBuffer> arrayBuffer) {
@@ -367,7 +354,6 @@ class V8TrackArrayBuffer : public CefTrackNode {
 
  private:
   v8::Isolate* isolate_;
-  void* buffer_;
   CefRefPtr<CefV8ArrayBufferReleaseCallback> release_callback_;
 };
 
@@ -1414,17 +1400,22 @@ CefRefPtr<CefV8Value> CefV8Value::CreateArrayBuffer(
   // Create a tracker object that will cause the user data reference to be
   // released when the V8 object is destroyed.
   V8TrackArrayBuffer* tracker =
-      new V8TrackArrayBuffer(isolate, buffer, release_callback);
+      new V8TrackArrayBuffer(isolate, release_callback);
+
+  if (release_callback)
+    release_callback->AddRef();
 
   auto deleter = [](void* data, size_t length, void* deleter_data) {
-    auto* tracker = reinterpret_cast<V8TrackArrayBuffer*>(deleter_data);
-    if (tracker) {
-      tracker->ReleaseBuffer();
+    auto* release_callback =
+        reinterpret_cast<CefV8ArrayBufferReleaseCallback*>(deleter_data);
+    if (release_callback) {
+      release_callback->ReleaseBuffer(data);
+      release_callback->Release();
     }
   };
 
-  std::unique_ptr<v8::BackingStore> backing =
-      v8::ArrayBuffer::NewBackingStore(buffer, length, deleter, tracker);
+  std::unique_ptr<v8::BackingStore> backing = v8::ArrayBuffer::NewBackingStore(
+      buffer, length, deleter, release_callback.get());
   v8::Local<v8::ArrayBuffer> ab =
       v8::ArrayBuffer::New(isolate, std::move(backing));
 
@@ -2302,8 +2293,6 @@ bool CefV8ValueImpl::NeuterArrayBuffer() {
     return false;
   }
   arr->Detach();
-  V8TrackArrayBuffer* tracker = V8TrackArrayBuffer::Unwrap(context, obj);
-  tracker->Detach();
 
   return true;
 }
diff --git a/tests/ceftests/v8_unittest.cc b/tests/ceftests/v8_unittest.cc
index f6aca3dc2..95d22633d 100644
--- a/tests/ceftests/v8_unittest.cc
+++ b/tests/ceftests/v8_unittest.cc
@@ -547,48 +547,32 @@ class V8RendererTest : public ClientAppRenderer::Delegate,
 
     bool destructorCalled = false;
     bool releaseBufferCalled = false;
-
-    bool neuteredDestructorCalled = false;
-    bool neuteredReleaseBufferCalled = false;
     // Enter the V8 context.
     EXPECT_TRUE(context->Enter());
     {
       int static_data[16];
-      CefRefPtr<CefV8Value> value;
       CefRefPtr<TestArrayBufferReleaseCallback> release_callback =
           new TestArrayBufferReleaseCallback(&destructorCalled,
                                              &releaseBufferCalled);
-
-      CefRefPtr<CefV8Value> neuteredValue;
-      CefRefPtr<TestArrayBufferReleaseCallback> neuteredReleaseCallback =
-          new TestArrayBufferReleaseCallback(&neuteredDestructorCalled,
-                                             &neuteredReleaseBufferCalled);
-      value = CefV8Value::CreateArrayBuffer(static_data, sizeof(static_data),
-                                            release_callback);
-      neuteredValue = CefV8Value::CreateArrayBuffer(
-          static_data, sizeof(static_data), neuteredReleaseCallback);
+      CefRefPtr<CefV8Value> value = CefV8Value::CreateArrayBuffer(
+          static_data, sizeof(static_data), release_callback);
       EXPECT_TRUE(value.get());
       EXPECT_TRUE(value->IsArrayBuffer());
       EXPECT_TRUE(value->IsObject());
       EXPECT_FALSE(value->HasValue(0));
-      EXPECT_FALSE(destructorCalled);
       EXPECT_TRUE(value->GetArrayBufferReleaseCallback().get() != nullptr);
       EXPECT_TRUE(((TestArrayBufferReleaseCallback*)value
                        ->GetArrayBufferReleaseCallback()
                        .get()) == release_callback);
 
-      // |neuteredValue| buffer is explicitly freed by NeuterArrayBuffer().
-      EXPECT_FALSE(neuteredReleaseBufferCalled);
-      EXPECT_TRUE(neuteredValue->NeuterArrayBuffer());
-      EXPECT_TRUE(neuteredReleaseBufferCalled);
-
-      // |value| buffer is implicitly freed when the value goes out of scope.
+      // |Value| buffer is explicitly freed by NeuterArrayBuffer().
+      EXPECT_FALSE(destructorCalled);
       EXPECT_FALSE(releaseBufferCalled);
+      EXPECT_TRUE(value->NeuterArrayBuffer());
+      EXPECT_TRUE(releaseBufferCalled);
     }
     // Exit the V8 context.
     EXPECT_TRUE(destructorCalled);
-    EXPECT_TRUE(releaseBufferCalled);
-    EXPECT_TRUE(neuteredDestructorCalled);
     EXPECT_TRUE(context->Exit());
     DestroyTest();
   }