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 <[email protected]> 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 <[email protected]> Commit-Queue: Evan Stade <[email protected]> Reviewed-by: Eriko Kurimoto <[email protected]> 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