BlobDispatcherHost: don't rely on NOTREACHED() checks in ParamTraits::Read

content/browser/blob_storage/blob_dispatcher_host.cc

Index: content/browser/blob_storage/blob_dispatcher_host.cc
diff --git a/content/browser/blob_storage/blob_dispatcher_host.cc b/content/browser/blob_storage/blob_dispatcher_host.cc
index 4f2ed8439f15ee20201f45bef45f5b836ccfb62f..1cd18d03b4364d8898c1e243689362974835c632 100644
--- a/content/browser/blob_storage/blob_dispatcher_host.cc
+++ b/content/browser/blob_storage/blob_dispatcher_host.cc
@@ -150,26 +150,46 @@ void BlobDispatcherHost::OnStartBuildingBlob(
   ChildProcessSecurityPolicyImpl* security_policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
   for (const DataElement& item : descriptions) {
-    if (item.type() == storage::DataElement::TYPE_FILE_FILESYSTEM) {
-      FileSystemURL filesystem_url(
-          file_system_context_->CrackURL(item.filesystem_url()));
-      if (!FileSystemURLIsValid(file_system_context_.get(), filesystem_url) ||
-          !security_policy->CanReadFileSystemFile(process_id_,
-                                                  filesystem_url)) {
-        async_builder_.CancelBuildingBlob(
-            uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED, context);
-        Send(new BlobStorageMsg_CancelBuildingBlob(
-            uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED));
-        return;
+    // For each source object that provides the data for the blob, ensure that
+    // this process has permission to read it.
+    switch (item.type()) {
+      case storage::DataElement::TYPE_FILE_FILESYSTEM: {
+        FileSystemURL filesystem_url(
+            file_system_context_->CrackURL(item.filesystem_url()));
+        if (!FileSystemURLIsValid(file_system_context_.get(), filesystem_url) ||
+            !security_policy->CanReadFileSystemFile(process_id_,
+                                                    filesystem_url)) {
+          async_builder_.CancelBuildingBlob(
+              uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED, context);
+          Send(new BlobStorageMsg_CancelBuildingBlob(
+              uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED));
+          return;
+        }
+        break;
+      }
+      case storage::DataElement::TYPE_FILE: {
+        if (!security_policy->CanReadFile(process_id_, item.path())) {
+          async_builder_.CancelBuildingBlob(
+              uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED, context);
+          Send(new BlobStorageMsg_CancelBuildingBlob(
+              uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED));
+          return;
+        }
+        break;
+      }
+      case storage::DataElement::TYPE_BLOB:
+      case storage::DataElement::TYPE_BYTES_DESCRIPTION:
+      case storage::DataElement::TYPE_BYTES: {
+        // Bytes are already in hand; no need to check read permission.
+        // TODO(nick): For TYPE_BLOB, can we actually get here for blobs
+        // originally created by other processes? If so, is that cool?
+        break;
+      }
+      case storage::DataElement::TYPE_UNKNOWN:
+      case storage::DataElement::TYPE_DISK_CACHE_ENTRY: {
+        CHECK(false);  // Should have been caught by IPC deserialization.

[dcheng, 2016/09/28 23:46:36]

I'm uncertain how I feel about CHECKing this. This is something that the
ParamTraits catches, since we're in an IPC handler, so why not just DCHECK here?

[ncarter (slow), 2016/10/05 23:18:46]

Done.

+        break;
       }
-    }
-    if (item.type() == storage::DataElement::TYPE_FILE &&
-        !security_policy->CanReadFile(process_id_, item.path())) {
-      async_builder_.CancelBuildingBlob(
-          uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED, context);
-      Send(new BlobStorageMsg_CancelBuildingBlob(
-          uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED));
-      return;
     }
   }