Chrome · FileAPI
CVE-2026-118331
Input Validation in FileAPI
Overview
Critical
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Changed Functions
| Function | Change | Notes |
|---|---|---|
ifstorage/browser/blob/blob_data_builder.cc |
modified | |
num_building_dependent_blobsstorage/browser/blob/blob_entry.cc |
modified | |
ifstorage/browser/blob/blob_entry.cc |
modified | |
forstorage/browser/blob/blob_entry.cc |
modified | |
ShareableBlobDataItemstorage/browser/blob/blob_entry.h |
modified | |
COMPONENT_EXPORTstorage/browser/blob/blob_entry.h |
modified | |
forstorage/browser/blob/blob_storage_context.cc |
modified |
Files Changed
storage/browser/blob/blob_data_handle.ccstorage/browser/blob/blob_storage_context.hstorage/browser/blob/blob_storage_context.ccstorage/browser/blob/blob_storage_context_unittest.ccstorage/browser/blob/blob_data_builder.ccstorage/browser/blob/view_blob_internals_job.ccstorage/browser/blob/blob_entry.ccstorage/browser/blob/blob_flattener_unittest.ccstorage/browser/blob/blob_storage_registry.hstorage/browser/blob/blob_entry.hstorage/browser/blob/blob_slice_unittest.ccstorage/browser/blob/blob_storage_registry.ccstorage/browser/blob/blob_storage_registry_unittest.ccstorage/browser/blob/blob_data_builder.h
Patch
From 136b3f96d1685548e2fa11fb57a667f6dbdd7087 Mon Sep 17 00:00:00 2001
From: Evan Stade <evanstade@microsoft.com>
Date: Wed, 21 Oct 2026 13:19:50 -0700
Subject: [PATCH] Avoid UAFs in BlobStorageContext
Many different flows could lead to a UAF of a BlobEntry*. The basic
story is:
1. BlobEntry A transitively holds some BlobDataHandle for B
2. Some operation on A, such as FinishBuilding, drops said handle for B
3. B has no more handles and stops using memory, freeing up memory for
some other BlobDataHandle C which had been blocked on quota
4. error occurs while C is acting on this, C self destructs
5. turns out C was the only thing holding A alive; A is destroyed
6. the rest of FinishLoading (or another method) continues, and doesn't
realize `BlobEntry* a` is now deleted.
A small twist on this story: `ClearAndFreeMemory(entry)` was also
re-entrant, because it can trigger a self-deletion similar to the above,
where `ClearAndFreeMemory` is called for the second time in
`DecrementBlobRefCount`. But `ClearAndFreeMemory` is mostly unnecessary
when the BlobEntry is already being deleted, so don't call it explicitly
on `DecrementBlobRefCount`, instead making it effectively part of
`~BlobEntry`.
Since this is a broad class of issues, it's not easy to spot-fix to
produce a small change. Instead of pervasively passing around
`BlobEntry*`, pass around `scoped_refptr<BlobEntry>`, so that it won't
be deleted out from under other operations.
Confusingly, `BlobDataHandle` is already a sort of ref counting, and
`BlobEntry` contains symbol names like `ref_count` and such; those are
renamed to "handle count" instead to avoid confusing with
`base::RefCounted`.
Bug: 572193211
Change-Id: I3d1747482ccfab8ccbff79a49219394c014789c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8620915
Reviewed-by: Fergal Daly <fergal@chromium.org>
Commit-Queue: Evan Stade <evanstade@microsoft.com>
Reviewed-by: Eriko Kurimoto <elkurin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1724068}
---
diff --git a/storage/browser/blob/blob_data_builder.cc b/storage/browser/blob/blob_data_builder.cc
index c058baa..ab8f165 100644
--- a/storage/browser/blob/blob_data_builder.cc
+++ b/storage/browser/blob/blob_data_builder.cc
@@ -174,7 +174,7 @@
uint64_t length,
const BlobStorageRegistry& blob_registry) {
DCHECK_GT(length, 0ul);
- const BlobEntry* ref_entry = blob_registry.GetEntry(uuid);
+ scoped_refptr<const BlobEntry> ref_entry = blob_registry.GetEntry(uuid);
// Self-references or non-existing blob references are invalid.
if (!ref_entry || uuid == uuid_) {
@@ -242,10 +242,10 @@
return;
}
- SliceBlob(ref_entry, offset, length);
+ SliceBlob(std::move(ref_entry), offset, length);
}
-void BlobDataBuilder::SliceBlob(const BlobEntry* source,
+void BlobDataBuilder::SliceBlob(scoped_refptr<const BlobEntry> source,
uint64_t slice_offset,
uint64_t slice_size) {
const auto& source_items = source->items();
diff --git a/storage/browser/blob/blob_data_builder.h b/storage/browser/blob/blob_data_builder.h
index c0dc7aa2..0a0b0a2 100644
--- a/storage/browser/blob/blob_data_builder.h
+++ b/storage/browser/blob/blob_data_builder.h
@@ -248,7 +248,7 @@
::std::ostream* os);
friend class BlobSliceTest;
- void SliceBlob(const BlobEntry* entry,
+ void SliceBlob(scoped_refptr<const BlobEntry> entry,
uint64_t slice_offset,
uint64_t slice_size);
diff --git a/storage/browser/blob/blob_data_handle.cc b/storage/browser/blob/blob_data_handle.cc
index 777aa9e..1ba116f 100644
--- a/storage/browser/blob/blob_data_handle.cc
+++ b/storage/browser/blob/blob_data_handle.cc
@@ -35,7 +35,7 @@
content_type_(content_type),
size_(size),
context_(context->AsWeakPtr()) {
- context_->IncrementBlobRefCount(uuid);
+ context_->IncrementBlobHandleCount(uuid);
}
std::unique_ptr<BlobReader> BlobDataHandle::CreateReader() const {
@@ -44,7 +44,7 @@
BlobDataHandle::BlobDataHandleShared::~BlobDataHandleShared() {
if (context_.get())
- context_->DecrementBlobRefCount(uuid_);
+ context_->DecrementBlobHandleCount(uuid_);
}
BlobDataHandle::BlobDataHandle(const std::string& uuid,
diff --git a/storage/browser/blob/blob_entry.cc b/storage/browser/blob/blob_entry.cc
index 716e9c7..336eab6 100644
--- a/storage/browser/blob/blob_entry.cc
+++ b/storage/browser/blob/blob_entry.cc
@@ -2,13 +2,15 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "storage/browser/blob/blob_entry.h"
+
#include <utility>
#include "base/functional/callback.h"
#include "base/metrics/histogram.h"
+#include "base/task/sequenced_task_runner.h"
#include "storage/browser/blob/blob_data_handle.h"
#include "storage/browser/blob/blob_data_item.h"
-#include "storage/browser/blob/blob_entry.h"
#include "storage/browser/blob/shareable_blob_data_item.h"
namespace storage {
@@ -35,17 +37,19 @@
num_building_dependent_blobs(num_building_dependent_blobs) {}
BlobEntry::BuildingState::~BuildingState() {
- DCHECK(!copy_quota_request);
- DCHECK(!transport_quota_request);
+ CancelRequestsAndAbort();
}
void BlobEntry::BuildingState::CancelRequestsAndAbort() {
- if (copy_quota_request)
+ if (copy_quota_request) {
copy_quota_request->Cancel();
- if (transport_quota_request)
+ }
+ if (transport_quota_request) {
transport_quota_request->Cancel();
- if (build_aborted_callback)
+ }
+ if (build_aborted_callback) {
std::move(build_aborted_callback).Run();
+ }
}
BlobEntry::BlobEntry(const std::string& content_type)
@@ -85,12 +89,33 @@
return items_;
}
-void BlobEntry::ClearItems() {
- items_.clear();
-}
+void BlobEntry::OnCancelBuilding(BlobStatus reason) {
+ TransportAllowedCallback transport_allowed;
-void BlobEntry::ClearOffsets() {
+ if (building_state_) {
+ transport_allowed = std::move(building_state_->transport_allowed_callback);
+
+ if (status_ == BlobStatus::PENDING_CONSTRUCTION) {
+ auto runner = base::SequencedTaskRunner::GetCurrentDefault();
+ for (auto& callback : building_state_->build_started_callbacks) {
+ runner->PostTask(FROM_HERE,
+ base::BindOnce(std::move(callback), reason));
+ }
+ }
+
+ building_state_->CancelRequestsAndAbort();
+ building_state_->transport_items.clear();
+ }
+
+ items_.clear();
offsets_.clear();
+ size_ = 0;
+ status_ = reason;
+
+ if (transport_allowed) {
+ std::move(transport_allowed)
+ .Run(reason, std::vector<BlobMemoryController::FileCreationInfo>());
+ }
}
} // namespace storage
diff --git a/storage/browser/blob/blob_entry.h b/storage/browser/blob/blob_entry.h
index fb71270..423a7740 100644
--- a/storage/browser/blob/blob_entry.h
+++ b/storage/browser/blob/blob_entry.h
@@ -14,6 +14,7 @@
#include "base/component_export.h"
#include "base/functional/callback.h"
#include "base/memory/raw_ptr.h"
+#include "base/memory/ref_counted.h"
#include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h"
#include "storage/browser/blob/blob_memory_controller.h"
@@ -23,7 +24,8 @@
class ShareableBlobDataItem;
// Represents a blob in BlobStorageRegistry. Exported only for unit tests.
-class COMPONENT_EXPORT(STORAGE_BROWSER) BlobEntry {
+class COMPONENT_EXPORT(STORAGE_BROWSER) BlobEntry
+ : public base::RefCounted<BlobEntry> {
public:
using TransportAllowedCallback = base::OnceCallback<
void(BlobStatus, std::vector<BlobMemoryController::FileCreationInfo>)>;
@@ -70,11 +72,10 @@
void CancelRequestsAndAbort();
const bool transport_items_present;
- // We can have trasnport data that's either populated or unpopulated. If we
+ // We can have transport data that's either populated or unpopulated. If we
// need population, this is populated.
TransportAllowedCallback transport_allowed_callback;
- std::vector<raw_ptr<ShareableBlobDataItem, VectorExperimental>>
- transport_items;
+ std::vector<raw_ptr<ShareableBlobDataItem>> transport_items;
BuildAbortedCallback build_aborted_callback;
@@ -106,8 +107,6 @@
BlobEntry(const BlobEntry&) = delete;
BlobEntry& operator=(const BlobEntry&) = delete;
- ~BlobEntry();
-
// Appends the given shared blob data item to this object.
void AppendSharedBlobItem(scoped_refptr<ShareableBlobDataItem> item);
void SetSharedBlobItems(
@@ -123,7 +122,7 @@
BlobStatus status() const { return status_; }
- size_t refcount() const { return refcount_; }
+ size_t handle_count() const { return handle_count_; }
const std::string& content_type() const { return content_type_; }
@@ -140,22 +139,25 @@
protected:
friend class BlobStorageContext;
- void IncrementRefCount() { ++refcount_; }
- void DecrementRefCount() { --refcount_; }
+ void IncrementHandleCount() { ++handle_count_; }
+ void DecrementHandleCount() { --handle_count_; }
+
+ void OnCancelBuilding(BlobStatus reason);
void set_status(BlobStatus status) { status_ = status; }
void set_size(uint64_t size) { size_ = size; }
- void ClearItems();
- void ClearOffsets();
-
void set_building_state(std::unique_ptr<BuildingState> building_state) {
building_state_ = std::move(building_state);
}
private:
+ friend class base::RefCounted<BlobEntry>;
+
+ ~BlobEntry();
+
BlobStatus status_ = BlobStatus::PENDING_QUOTA;
- size_t refcount_ = 0;
+ size_t handle_count_ = 0;
// Metadata.
std::string content_type_;
diff --git a/storage/browser/blob/blob_storage_context.cc b/storage/browser/blob/blob_storage_context.cc
index a6f7813d..dd51665 100644
--- a/storage/browser/blob/blob_storage_context.cc
+++ b/storage/browser/blob/blob_storage_context.cc
@@ -73,10 +73,10 @@
std::unique_ptr<BlobDataHandle> BlobStorageContext::GetBlobDataFromUUID(
const std::string& uuid) {
- BlobEntry* entry = registry_.GetEntry(uuid);
+ scoped_refptr<BlobEntry> entry = registry_.GetEntry(uuid);
if (!entry)
return nullptr;
- return CreateHandle(uuid, entry);
+ return CreateHandle(uuid, std::move(entry));
}
void BlobStorageContext::GetBlobDataFromBlobRemote(
@@ -112,7 +112,7 @@
const std::string& content_type,
std::vector<scoped_refptr<ShareableBlobDataItem>> items) {
TRACE_EVENT0("Blob", "Context::AddFinishedBlobFromItems");
- BlobEntry* entry = registry_.CreateEntry(uuid, content_type);
+ scoped_refptr<BlobEntry> entry = registry_.CreateEntry(uuid, content_type);
for (const auto& item : items) {
DCHECK_EQ(item->state(), ShareableBlobDataItem::POPULATED_WITH_QUOTA);
DCHECK_NE(BlobDataItem::Type::kBytesDescription, item->item()->type());
@@ -132,10 +132,10 @@
BlobStatus reason) {
DCHECK(!registry_.HasEntry(uuid));
DCHECK(BlobStatusIsError(reason));
- BlobEntry* entry = registry_.CreateEntry(uuid, content_type);
+ scoped_refptr<BlobEntry> entry = registry_.CreateEntry(uuid, content_type);
entry->set_status(reason);
FinishBuilding(entry);
- return CreateHandle(uuid, entry);
+ return CreateHandle(uuid, std::move(entry));
}
std::unique_ptr<BlobDataHandle> BlobStorageContext::AddFutureBlob(
@@ -144,25 +144,24 @@
BuildAbortedCallback build_aborted_callback) {
DCHECK(!registry_.HasEntry(uuid));
- BlobEntry* entry = registry_.CreateEntry(uuid, content_type);
+ scoped_refptr<BlobEntry> entry = registry_.CreateEntry(uuid, content_type);
entry->set_size(blink::BlobUtils::kUnknownSize);
entry->set_status(BlobStatus::PENDING_CONSTRUCTION);
entry->set_building_state(std::make_unique<BlobEntry::BuildingState>(
false, TransportAllowedCallback(), 0));
entry->building_state_->build_aborted_callback =
std::move(build_aborted_callback);
- return CreateHandle(uuid, entry);
+ return CreateHandle(uuid, std::move(entry));
}
std::unique_ptr<BlobDataHandle> BlobStorageContext::BuildPreregisteredBlob(
std::unique_ptr<BlobDataBuilder> content,
TransportAllowedCallback transport_allowed_callback) {
- BlobEntry* entry = registry_.GetEntry(content->uuid());
- DCHECK(entry);
+ scoped_refptr<BlobEntry> entry = registry_.GetEntry(content->uuid());
DCHECK_EQ(BlobStatus::PENDING_CONSTRUCTION, entry->status());
entry->set_size(0);
- return BuildBlobInternal(entry, std::move(content),
+ return BuildBlobInternal(std::move(entry), std::move(content),
std::move(transport_allowed_callback));
}
@@ -171,15 +170,15 @@
TransportAllowedCallback transport_allowed_callback) {
DCHECK(!registry_.HasEntry(content->uuid_));
- BlobEntry* entry =
+ scoped_refptr<BlobEntry> entry =
registry_.CreateEntry(content->uuid(), content->content_type_);
- return BuildBlobInternal(entry, std::move(content),
+ return BuildBlobInternal(std::move(entry), std::move(content),
std::move(transport_allowed_callback));
}
std::unique_ptr<BlobDataHandle> BlobStorageContext::BuildBlobInternal(
- BlobEntry* entry,
+ scoped_refptr<BlobEntry> entry,
std::unique_ptr<BlobDataBuilder> content,
TransportAllowedCallback transport_allowed_callback) {
#if DCHECK_IS_ON()
@@ -260,8 +259,8 @@
// data item, and we need to account for that. So we prevent that case by
// holding onto all blobs.
for (const std::string& dependent_blob_uuid : content->dependent_blobs()) {
- BlobEntry* blob_entry = registry_.GetEntry(dependent_blob_uuid);
- DCHECK(blob_entry);
+ scoped_refptr<BlobEntry> blob_entry =
+ registry_.GetEntry(dependent_blob_uuid);
if (BlobStatusIsError(blob_entry->status())) {
entry->set_status(BlobStatus::ERR_REFERENCED_BLOB_BROKEN);
break;
@@ -275,8 +274,7 @@
}
}
- std::vector<raw_ptr<ShareableBlobDataItem, VectorExperimental>>
- transport_items;
+ std::vector<raw_ptr<ShareableBlobDataItem>> transport_items;
transport_items.reserve(content->pending_transport_items().size());
for (const auto& item : content->pending_transport_items())
transport_items.emplace_back(item.get());
@@ -377,10 +375,10 @@
}
void BlobStorageContext::NotifyTransportComplete(const std::string& uuid) {
- BlobEntry* entry = registry_.GetEntry(uuid);
+ scoped_refptr<BlobEntry> entry = registry_.GetEntry(uuid);
CHECK(entry) << "There is no blob entry with uuid " << uuid;
DCHECK(BlobStatusIsPending(entry->status()));
- NotifyTransportCompleteInternal(entry);
+ NotifyTransportCompleteInternal(std::move(entry));
}
... (file diff truncated)
Loading diff…
Regression Test / PoC
shipped with the fix
diff --git a/storage/browser/blob/blob_flattener_unittest.cc b/storage/browser/blob/blob_flattener_unittest.cc
index 4c2b5ea..8225d5b 100644
--- a/storage/browser/blob/blob_flattener_unittest.cc
+++ b/storage/browser/blob/blob_flattener_unittest.cc
@@ -96,7 +96,7 @@
const ShareableBlobDataItem& GetItemInBlob(const std::string& uuid,
size_t index) {
- const BlobEntry* entry = registry().GetEntry(uuid);
+ scoped_refptr<const BlobEntry> entry = registry().GetEntry(uuid);
EXPECT_TRUE(entry);
return *entry->items()[index];
}
diff --git a/storage/browser/blob/blob_slice_unittest.cc b/storage/browser/blob/blob_slice_unittest.cc
index a1684a4..563687ec 100644
--- a/storage/browser/blob/blob_slice_unittest.cc
+++ b/storage/browser/blob/blob_slice_unittest.cc
@@ -53,10 +53,10 @@
}
void Slice(BlobDataBuilder& builder,
- BlobEntry* source,
+ scoped_refptr<BlobEntry> source,
uint64_t slice_offset,
uint64_t slice_size) {
- builder.SliceBlob(source, slice_offset, slice_size);
+ builder.SliceBlob(std::move(source), slice_offset, slice_size);
}
void ExpectSlice(scoped_refptr<ShareableBlobDataItem> dest_item,
@@ -76,12 +76,12 @@
TEST_F(BlobSliceTest, FullItem) {
const size_t kSize = 5u;
- BlobEntry data(kType);
+ auto data = base::MakeRefCounted<BlobEntry>(kType);
scoped_refptr<ShareableBlobDataItem> item = CreateDataItem(kSize);
- data.AppendSharedBlobItem(item);
+ data->AppendSharedBlobItem(item);
BlobDataBuilder builder(kId);
- Slice(builder, &data, 0, 5);
+ Slice(builder, data, 0, 5);
EXPECT_TRUE(builder.copies().empty());
ASSERT_EQ(1u, builder.items().size());
EXPECT_EQ(item, builder.items()[0]);
@@ -90,12 +90,12 @@
TEST_F(BlobSliceTest, SliceSingleItem) {
const size_t kSize = 5u;
- BlobEntry data(kType);
+ auto data = base::MakeRefCounted<BlobEntry>(kType);
scoped_refptr<ShareableBlobDataItem> item = CreateDataItem(kSize);
- data.AppendSharedBlobItem(item);
+ data->AppendSharedBlobItem(item);
BlobDataBuilder builder(kId);
- Slice(builder, &data, 1, 3);
+ Slice(builder, data, 1, 3);
ASSERT_EQ(1u, builder.copies().size());
ASSERT_EQ(1u, builder.items().size());
@@ -106,14 +106,14 @@
const size_t kSize1 = 5u;
const size_t kSize2 = 10u;
- BlobEntry data(kType);
+ auto data = base::MakeRefCounted<BlobEntry>(kType);
scoped_refptr<ShareableBlobDataItem> item1 = CreateDataItem(kSize1);
scoped_refptr<ShareableBlobDataItem> item2 = CreateDataItem(kSize2);
- data.AppendSharedBlobItem(item1);
- data.AppendSharedBlobItem(item2);
+ data->AppendSharedBlobItem(item1);
+ data->AppendSharedBlobItem(item2);
BlobDataBuilder builder(kId);
- Slice(builder, &data, 6, 2);
+ Slice(builder, data, 6, 2);
ASSERT_EQ(1u, builder.copies().size());
ASSERT_EQ(1u, builder.items().size());
@@ -124,14 +124,14 @@
const size_t kSize1 = 5u;
const size_t kSize2 = 10u;
- BlobEntry data(kType);
+ auto data = base::MakeRefCounted<BlobEntry>(kType);
scoped_refptr<ShareableBlobDataItem> item1 = CreateDataItem(kSize1);
scoped_refptr<ShareableBlobDataItem> item2 = CreateDataItem(kSize2);
- data.AppendSharedBlobItem(item1);
- data.AppendSharedBlobItem(item2);
+ data->AppendSharedBlobItem(item1);
+ data->AppendSharedBlobItem(item2);
BlobDataBuilder builder(kId);
- Slice(builder, &data, 4, 10);
+ Slice(builder, data, 4, 10);
ASSERT_EQ(2u, builder.copies().size());
ASSERT_EQ(2u, builder.items().size());
@@ -143,14 +143,14 @@
const size_t kSize1 = 5u;
const size_t kSize2 = 10u;
- BlobEntry data(kType);
+ auto data = base::MakeRefCounted<BlobEntry>(kType);
scoped_refptr<ShareableBlobDataItem> item1 = CreateFileItem(0u, kSize1);
scoped_refptr<ShareableBlobDataItem> item2 = CreateDataItem(kSize2);
- data.AppendSharedBlobItem(item1);
- data.AppendSharedBlobItem(item2);
+ data->AppendSharedBlobItem(item1);
+ data->AppendSharedBlobItem(item2);
BlobDataBuilder builder(kId);
- Slice(builder, &data, 4, 2);
+ Slice(builder, data, 4, 2);
ASSERT_EQ(1u, builder.copies().size());
ASSERT_EQ(2u, builder.items().size());
@@ -163,16 +163,16 @@
const size_t kSize2 = 10u;
const size_t kSize3 = 10u;
- BlobEntry data(kType);
+ auto data = base::MakeRefCounted<BlobEntry>(kType);
scoped_refptr<ShareableBlobDataItem> item1 = CreateDataItem(kSize1);
scoped_refptr<ShareableBlobDataItem> item2 = CreateFileItem(0u, kSize2);
scoped_refptr<ShareableBlobDataItem> item3 = CreateDataItem(kSize3);
- data.AppendSharedBlobItem(item1);
- data.AppendSharedBlobItem(item2);
- data.AppendSharedBlobItem(item3);
+ data->AppendSharedBlobItem(item1);
+ data->AppendSharedBlobItem(item2);
+ data->AppendSharedBlobItem(item3);
BlobDataBuilder builder(kId);
- Slice(builder, &data, 2, 20);
+ Slice(builder, data, 2, 20);
ASSERT_EQ(2u, builder.copies().size());
ASSERT_EQ(3u, builder.items().size());
@@ -182,12 +182,12 @@
}
TEST_F(BlobSliceTest, SliceTempFileItem) {
- BlobEntry data(kType);
+ auto data = base::MakeRefCounted<BlobEntry>(kType);
scoped_refptr<ShareableBlobDataItem> item1 = CreateTempFileItem(1u, 10u);
- data.AppendSharedBlobItem(item1);
+ data->AppendSharedBlobItem(item1);
BlobDataBuilder builder(kId);
- Slice(builder, &data, 2, 5);
+ Slice(builder, data, 2, 5);
ASSERT_EQ(1u, builder.copies().size());
ASSERT_EQ(1u, builder.items().size());
auto dest_item = builder.items()[0];
diff --git a/storage/browser/blob/blob_storage_context_unittest.cc b/storage/browser/blob/blob_storage_context_unittest.cc
index 7f25ee77..9c49844 100644
--- a/storage/browser/blob/blob_storage_context_unittest.cc
+++ b/storage/browser/blob/blob_storage_context_unittest.cc
@@ -22,6 +22,7 @@
#include "base/run_loop.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/stringprintf.h"
+#include "base/test/bind.h"
#include "base/test/task_environment.h"
#include "base/test/test_simple_task_runner.h"
#include "base/threading/thread_restrictions.h"
@@ -114,12 +115,12 @@
context_->mutable_memory_controller()->set_limits_for_testing(limits);
}
- void IncrementRefCount(const std::string& uuid) {
- context_->IncrementBlobRefCount(uuid);
+ void IncrementHandleCount(const std::string& uuid) {
+ context_->IncrementBlobHandleCount(uuid);
}
- void DecrementRefCount(const std::string& uuid) {
- context_->DecrementBlobRefCount(uuid);
+ void DecrementHandleCount(const std::string& uuid) {
+ context_->DecrementBlobHandleCount(uuid);
}
std::string UUIDFromBlob(blink::mojom::Blob* blob) {
@@ -301,16 +302,16 @@
std::unique_ptr<BlobDataHandle> blob_data_handle = SetupBasicBlob(kId);
// Do an extra increment to keep it around after we kill the handle.
- IncrementRefCount(kId);
- IncrementRefCount(kId);
- DecrementRefCount(kId);
+ IncrementHandleCount(kId);
+ IncrementHandleCount(kId);
+ DecrementHandleCount(kId);
blob_data_handle = context_->GetBlobDataFromUUID(kId);
EXPECT_TRUE(blob_data_handle);
blob_data_handle.reset();
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(context_->registry().HasEntry(kId));
- DecrementRefCount(kId);
+ DecrementHandleCount(kId);
EXPECT_FALSE(context_->registry().HasEntry(kId));
// Make sure it goes away in the end.
@@ -972,6 +973,147 @@
EXPECT_EQ(0u, context_->memory_controller().memory_usage());
}
+// Regression test for crbug.com/572193211
+TEST_F(BlobStorageContextTest, FinishBuildingWontUafEntry) {
+ context_ = std::make_unique<BlobStorageContext>(
+ temp_dir_.GetPath(), temp_dir_.GetPath(), file_runner_);
+ SetTestMemoryLimits();
+
+ const std::string kIdB("id_b");
+ const std::string kIdA("id_a");
+ const std::string kIdC("id_c");
+
+ const size_t a_future_size = kTestBlobStorageMaxBlobMemorySize / 10;
+ const size_t b_future_size = kTestBlobStorageMaxBlobMemorySize * 3 / 4;
+ const size_t c_future_size = kTestBlobStorageMaxBlobMemorySize / 2;
+
+ // 1. Start building Blob B with 300 bytes of future data. Because B is not
+ // yet populated, its 300 bytes count toward `memory_usage()` (via
+ // `blob_memory_used_`) without being eligible for eviction to disk
+ // (`in_flight_memory_used_` stays 0).
+ auto builder_b = std::make_unique<BlobDataBuilder>(kIdB);
+ BlobDataBuilder::FutureData future_data_b =
+ builder_b->AppendFutureData(b_future_size);
+ BlobStatus status_b = BlobStatus::ERR_INVALID_CONSTRUCTION_ARGUMENTS;
+ std::unique_ptr<BlobDataHandle> handle_b = context_->BuildBlob(
+ std::move(builder_b),
+ base::BindOnce(&SaveBlobStatusAndFiles, &status_b, &files_));
+ EXPECT_EQ(BlobStatus::PENDING_TRANSPORT, status_b);
+ EXPECT_EQ(300u, context_->memory_controller().memory_usage());
+
+ // 2. Start building Blob A, which slices from B, so A's
+ // `building_state_->dependent_blobs` holds a handle to B.
+ auto builder_a = std::make_unique<BlobDataBuilder>(kIdA);
+ builder_a->AppendBlob(kIdB, 0, 10, context_->registry());
+ BlobDataBuilder::FutureData future_data_a =
+ builder_a->AppendFutureData(a_future_size);
+ BlobStatus status_a = BlobStatus::ERR_INVALID_CONSTRUCTION_ARGUMENTS;
+ std::unique_ptr<BlobDataHandle> handle_a = context_->BuildBlob(
+ std::move(builder_a),
+ base::BindOnce(&SaveBlobStatusAndFiles, &status_a, &files_));
+ EXPECT_EQ(BlobStatus::PENDING_TRANSPORT, status_a);
+
+ // Drop external handle to B; now ONLY A's `building_state_->dependent_blobs`
+ // keeps B (and its 300-byte MemoryAllocation) alive.
+ handle_b.reset();
+
+ // 3. Start building Blob C, which depends on A and requests 200 bytes of
+ // transport quota. Because 300 + 20 + 200 > 400, C's quota request is
+ // queued in `pending_memory_quota_tasks_`.
+ auto builder_c = std::make_unique<BlobDataBuilder>(kIdC);
+ builder_c->AppendBlob(kIdA, 0, 10, context_->registry());
+ builder_c->AppendFutureData(c_future_size);
+ std::unique_ptr<BlobDataHandle> handle_c = context_->BuildBlob(
+ std::move(builder_c),
+ base::BindLambdaForTesting(
+ [&](BlobStatus status, std::vector<FileCreationInfo> /*files*/) {
+ if (status == BlobStatus::PENDING_TRANSPORT) {
+ // Simulate transport setup failure (e.g. data pipe creation OOM).
+ // Canceling C resets C's `building_state_`, dropping the last
+ // handle to A synchronously while A is inside `FinishBuilding`.
+ context_->CancelBuildingBlob(kIdC, BlobStatus::ERR_OUT_OF_MEMORY);
+ }
+ }));
+ EXPECT_EQ(BlobStatus::PENDING_QUOTA, handle_c->GetBlobStatus());
+
+ // Drop external handle to A; now ONLY C's `building_state_->dependent_blobs`
+ // keeps A alive. Keep `handle_c` alive so C stays in the registry while
+ // waiting for quota.
+ handle_a.reset();
+ EXPECT_TRUE(context_->registry().HasEntry(kIdA));
+
+ // 4. Cancel A. Inside `FinishBuilding(A)`:
+ // - `A->building_state_.reset()` drops A's handle to B.
+ // - B is deleted, freeing its 300-byte allocation and synchronously
+ // granting C's 200-byte quota request.
+ // - C's `transport_allowed_callback` runs and cancels C.
+ // - C's `building_state_.reset()` drops the last handle to A, deleting A!
+ context_->CancelBuildingBlob(kIdA, BlobStatus::ERR_OUT_OF_MEMORY);
+ EXPECT_FALSE(context_->registry().HasEntry(kIdA));
+}
+
+// Regression test for crbug.com/572193211
+TEST_F(BlobStorageContextTest, CancelBuildingBlobWontUafEntry) {
+ context_ = std::make_unique<BlobStorageContext>(
+ temp_dir_.GetPath(), temp_dir_.GetPath(), file_runner_);
+ SetTestMemoryLimits();
+
+ const std::string kIdA("id_a");
+ const std::string kIdC("id_c");
+
+ const size_t a_future_size = kTestBlobStorageMaxBlobMemorySize * 3 / 4;
... (truncated)
Loading diff…
Original Bug Report
The reporter's bug is still restricted on the tracker. Chrome de-restricts security bugs ~30–90 days after the fix ships; a later run will backfill it here.
References
On This Page