[Backport] Security bug 1221068

Cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/3103206:
[M90-LTS] NativeIO: Fix potential NativeIOHost lifetime issue.

See https://crbug.com/1221068#c16 for a description.

(cherry picked from commit 1737623a5311badc6396e65503f802d6267b10be)

Bug: 1221068
Change-Id: I93b62f39190519da1101f6a45009baf998516491
Commit-Queue: Victor Costan <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#908087}
Reviewed-by: Artem Sumaneev <[email protected]>
Owners-Override: Artem Sumaneev <[email protected]>
Commit-Queue: Zakhar Voit <[email protected]>
Cr-Commit-Position: refs/branch-heads/4430@{#1572}
Cr-Branched-From: e5ce7dc4f7518237b3d9bb93cccca35d25216cbe-refs/heads/master@{#857950}
Reviewed-by: Allan Sandfeld Jensen <[email protected]>

Author: zakharvoit

chromium/content/browser/native_io/native_io_host.cc

@@ -599,8 +599,11 @@ void NativeIOHost::DidDeleteAllData(base::File::Error error) {
       std::move(delete_all_data_callbacks_);
   delete_all_data_callbacks_.clear();
   for (DeleteAllDataCallback& callback : callbacks) {
-    std::move(callback).Run(error, this);
+    std::move(callback).Run(error);
   }
+
+  // May delete `this`.
+  manager_->DidDeleteHostData(this, base::PassKey<NativeIOHost>());
 }
 
 }  // namespace content

chromium/content/browser/native_io/native_io_host.h

@@ -38,8 +38,7 @@ class NativeIOFileHost;
 // sequences, if desired.
 class NativeIOHost : public blink::mojom::NativeIOHost {
  public:
-  using DeleteAllDataCallback =
-      base::OnceCallback<void(base::File::Error result, NativeIOHost* host)>;
+  using DeleteAllDataCallback = base::OnceCallback<void(base::File::Error)>;
 
   // `allow_set_length_ipc` gates NativeIOFileHost::SetLength(), which works
   // around a sandboxing limitation on macOS < 10.15. This is plumbed as a flag

chromium/content/browser/native_io/native_io_manager.cc

@@ -165,6 +165,15 @@ void NativeIOManager::OnHostReceiverDisconnect(NativeIOHost* host) {
   MaybeDeleteHost(host);
 }
 
+void NativeIOManager::DidDeleteHostData(NativeIOHost* host,
+                                        base::PassKey<NativeIOHost>) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(host != nullptr);
+  DCHECK(!host->delete_all_data_in_progress());
+
+  MaybeDeleteHost(host);
+}
+
 void NativeIOManager::MaybeDeleteHost(NativeIOHost* host) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(host != nullptr);
@@ -177,18 +186,6 @@ void NativeIOManager::MaybeDeleteHost(NativeIOHost* host) {
   hosts_.erase(host->origin());
 }
 
-void NativeIOManager::OnDeleteOriginDataCompleted(
-    storage::QuotaClient::DeleteOriginDataCallback callback,
-    base::File::Error result,
-    NativeIOHost* host) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  MaybeDeleteHost(host);
-  blink::mojom::QuotaStatusCode quota_result =
-      result == base::File::FILE_OK ? blink::mojom::QuotaStatusCode::kOk
-                                    : blink::mojom::QuotaStatusCode::kUnknown;
-  std::move(callback).Run(quota_result);
-}
-
 void NativeIOManager::DeleteOriginData(
     const url::Origin& origin,
     storage::QuotaClient::DeleteOriginDataCallback callback) {
@@ -224,12 +221,16 @@ void NativeIOManager::DeleteOriginData(
     DCHECK(insert_succeeded);
   }
 
-  // base::Unretained is safe here because this NativeIOManager owns the
-  // NativeIOHost. So, the unretained NativeIOManager is guaranteed to outlive
-  // the  NativeIOHost and the closure that it uses.
-  it->second->DeleteAllData(
-      base::BindOnce(&NativeIOManager::OnDeleteOriginDataCompleted,
-                     base::Unretained(this), std::move(callback)));
+  // DeleteAllData() will call DidDeleteHostData() asynchronously, which may
+  // delete this entry from `hosts_`.
+  it->second->DeleteAllData(base::BindOnce(
+      [](storage::mojom::QuotaClient::DeleteOriginDataCallback callback,
+         base::File::Error error) {
+        std::move(callback).Run((error == base::File::FILE_OK)
+                                    ? blink::mojom::QuotaStatusCode::kOk
+                                    : blink::mojom::QuotaStatusCode::kUnknown);
+      },
+      std::move(callback)));
 }
 
 void NativeIOManager::GetOriginsForType(

chromium/content/browser/native_io/native_io_manager.h

@@ -117,13 +117,10 @@ class CONTENT_EXPORT NativeIOManager {
   // NativeIOHost.
   void OnHostReceiverDisconnect(NativeIOHost* host);
 
-  // Callback function when DeleteOriginData has completed.
+  // Called when a NativeIOHost finishes processing a data deletion request.
   //
-  // `host` must be owned by this manager.
-  void OnDeleteOriginDataCompleted(
-      storage::QuotaClient::DeleteOriginDataCallback callback,
-      base::File::Error result,
-      NativeIOHost* host);
+  // `host` must be owned by this manager. `host` may be deleted.
+  void DidDeleteHostData(NativeIOHost* host, base::PassKey<NativeIOHost>);
 
   storage::QuotaManagerProxy* quota_manager_proxy() const {
     return quota_manager_proxy_.get();