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

content/browser/blob_storage/blob_dispatcher_host.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/blob_storage/blob_dispatcher_host.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 132 matching lines @@
     return;
   }
   if (!async_builder_.IsBeingBuilt(uuid)) {
     SendIPCResponse(uuid, BlobTransportResult::BAD_IPC);
     return;
   }
 
   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;
       }
-    }
-    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;
+      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;
+      }
     }
   }
 
   // |this| owns async_builder_ so using base::Unretained(this) is safe.
   BlobTransportResult result = async_builder_.StartBuildingBlob(
       uuid, descriptions, context->memory_available(), context,
       base::Bind(&BlobDispatcherHost::SendMemoryRequest, base::Unretained(this),
                  uuid));
   SendIPCResponse(uuid, result);
 }
@@ skipping 215 matching lines @@
     context->RevokePublicBlobURL(url);
   }
   for (const auto& uuid_refnum_pair : blobs_inuse_map_) {
     for (int i = 0; i < uuid_refnum_pair.second; ++i)
       context->DecrementBlobRefCount(uuid_refnum_pair.first);
   }
   async_builder_.CancelAll(context);
 }
 
 }  // namespace content