Make network::DataElement a union with absl::variant

network.mojom.DataElement represents part of a request body. It is a union of several types, and that applies to its typemapped class, network::DataElement. Up until now we had flat members (e.g., offset(), length(), bytes()) with various DCHECKs on network::DataElement. This led to several issues: 1) The code author can be confused. Some members (e.g., length(), bytes()) seems to have a meaning in a wrong context, and that can lead to security issues. See https://crbug.com/1151865 for example. 2) It is hard to maintain the DCHECKs in a consistent way. For example, at this moment we have a DCHECK at chunked_data_pipe_getter(), but we don't have a DCHECK at length(). We have a similar difficulty on mojo deserialization. This CL makes network::DataElement a simple wrapper of absl::variant. network::DataElement has only one member: absl::variant<absl::monostate, DataElementBytes, DataElementDataPipe, DataElementChunkedDataPipe, DataElementFile> variant_; A user needs to check the type and explicitly cast the element to the correct class, to access any information (other than `type`). We rely on checks implemented in absl::variant, so we don't need to have many DCHECKs. This CL also contains the following changes: A) DataElementType.kReadOnceStream is gone, as having two members sharing the same type makes the use of absl::variant difficult. This merges the type to DataElementType.kChunkedDataPipe, and introduced a boolean `read_only_once` in DataElement to distinguish them. B) Removed FetchAPIDataElement, as it seems identical to network.mojom.DataElement. C) Deserialization logic gets more strict. Invalid mojo::pending_remote for kDataPipe and kChunkedDataPipe are now treated as errors. D) Made network.mojom.DataElement a union. E) Added some deserialization tests for network::DataElement. Change-Id: I2c646ead57bc19bee661b711c9c863ce1a5e9c94 Bug: 1156550, 1151865, 1152664 Tbr: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected] Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2599731 Reviewed-by: Alex Ilin <[email protected]> Reviewed-by: Matt Menke <[email protected]> Reviewed-by: Daniel Cheng <[email protected]> Reviewed-by: Matt Falkenhagen <[email protected]> Reviewed-by: Nathan Parker <[email protected]> Commit-Queue: Yutaka Hirano <[email protected]> Cr-Commit-Position: refs/heads/master@{#842964}
yjfxfjch · Jan 13, 2021 · 69d394f · 69d394f
1 parent 6f1ad3e
commit 69d394f
Show file tree

Hide file tree

Showing 46 changed files with 887 additions and 758 deletions.
diff --git a/chrome/browser/autofill/automated_tests/cache_replayer.cc b/chrome/browser/autofill/automated_tests/cache_replayer.cc
@@ -218,11 +218,11 @@ std::string GetStringFromDataElements(
     const std::vector<network::DataElement>* data_elements) {
   std::string result;
   for (const network::DataElement& element : *data_elements) {
-    DCHECK_EQ(element.type(), network::mojom::DataElementType::kBytes);
+    DCHECK_EQ(element.type(), network::DataElement::Tag::kBytes);
     // Provide the length of the bytes explicitly, not to rely on the null
     // termination.
-    result.append(element.bytes(),
-                  base::checked_cast<size_t>(element.length()));
+    const auto piece = element.As<network::DataElementBytes>().AsStringPiece();
+    result.append(piece.data(), piece.size());
   }
   return result;
 }

diff --git a/chrome/browser/sharing/web_push/web_push_sender_unittest.cc b/chrome/browser/sharing/web_push/web_push_sender_unittest.cc
@@ -141,7 +141,8 @@ TEST_F(WebPushSenderTest, SendMessageTest) {
       pendingRequest->request.request_body->elements();
   ASSERT_EQ(1UL, body_elements->size());
   const network::DataElement& body = body_elements->back();
-  EXPECT_EQ("payload", std::string(body.bytes(), body.length()));
+  ASSERT_EQ(network::DataElement::Tag::kBytes, body.type());
+  EXPECT_EQ("payload", body.As<network::DataElementBytes>().AsStringPiece());
 
   auto response_head = network::CreateURLResponseHead(net::HTTP_OK);
   response_head->headers->AddHeader("location",

diff --git a/chrome/browser/ui/webui/reset_password/reset_password_ui.cc b/chrome/browser/ui/webui/reset_password/reset_password_ui.cc
@@ -89,10 +89,13 @@ PasswordType GetPasswordType(content::WebContents* web_contents) {
   if (!nav_entry || !nav_entry->GetHasPostData())
     return PasswordType::PASSWORD_TYPE_UNKNOWN;
   auto& post_data = nav_entry->GetPostData()->elements()->at(0);
-  int post_data_int = -1;
-  if (base::StringToInt(std::string(post_data.bytes(), post_data.length()),
-                        &post_data_int)) {
-    return static_cast<PasswordType>(post_data_int);
+  if (post_data.type() == network::DataElement::Tag::kBytes) {
+    int post_data_int = -1;
+    if (base::StringToInt(
+            post_data.As<network::DataElementBytes>().AsStringPiece(),
+            &post_data_int)) {
+      return static_cast<PasswordType>(post_data_int);
+    }
   }
 
   return PasswordType::PASSWORD_TYPE_UNKNOWN;

diff --git a/chrome/browser/web_share_target/target_util_unittest.cc b/chrome/browser/web_share_target/target_util_unittest.cc
@@ -11,18 +11,19 @@
 namespace web_share_target {
 
 std::string convertDataElementToString(const network::DataElement& element) {
-  if (element.type() == network::mojom::DataElementType::kBytes) {
-    return std::string(element.bytes(), element.length());
+  if (element.type() == network::DataElement::Tag::kBytes) {
+    return std::string(element.As<network::DataElementBytes>().AsStringPiece());
   }
-  if (element.type() == network::mojom::DataElementType::kFile) {
-    return std::string(element.path().AsUTF8Unsafe());
+  if (element.type() == network::DataElement::Tag::kFile) {
+    return std::string(
+        element.As<network::DataElementFile>().path().AsUTF8Unsafe());
   }
   return "";
 }
 
 void CheckDataElements(
     const scoped_refptr<network::ResourceRequestBody>& body,
-    const std::vector<network::mojom::DataElementType>& expected_element_types,
+    const std::vector<network::DataElement::Tag>& expected_element_types,
     const std::vector<std::string>& expected_element_values) {
   EXPECT_NE(nullptr, body->elements());
   const std::vector<network::DataElement>& data_elements = *body->elements();
@@ -67,12 +68,10 @@ TEST(TargetUtilTest, ValidMultipartBodyForFile) {
       ComputeMultipartBody(names, values, is_value_file_uris, filenames, types,
                            boundary);
 
-  std::vector<network::mojom::DataElementType> expected_types = {
-      network::mojom::DataElementType::kBytes,
-      network::mojom::DataElementType::kFile,
-      network::mojom::DataElementType::kBytes,
-      network::mojom::DataElementType::kBytes,
-      network::mojom::DataElementType::kBytes};
+  std::vector<network::DataElement::Tag> expected_types = {
+      network::DataElement::Tag::kBytes, network::DataElement::Tag::kFile,
+      network::DataElement::Tag::kBytes, network::DataElement::Tag::kBytes,
+      network::DataElement::Tag::kBytes};
   std::vector<std::string> expected = {
       "--boundary\r\nContent-Disposition: form-data; name=\"share-file%22\"; "
       "filename=\"filename%0D%0A\"\r\nContent-Type: type\r\n\r\n",
@@ -97,9 +96,8 @@ TEST(TargetUtilTest, ValidMultipartBodyForText) {
       ComputeMultipartBody(names, values, is_value_file_uris, filenames, types,
                            boundary);
 
-  std::vector<network::mojom::DataElementType> expected_types = {
-      network::mojom::DataElementType::kBytes,
-      network::mojom::DataElementType::kBytes};
+  std::vector<network::DataElement::Tag> expected_types = {
+      network::DataElement::Tag::kBytes, network::DataElement::Tag::kBytes};
   std::vector<std::string> expected = {
       "--boundary\r\nContent-Disposition: form-data; "
       "name=\"name%22\"\r\nContent-Type: type\r\n\r\nvalue\r\n",
@@ -126,27 +124,24 @@ TEST(TargetUtilTest, ValidMultipartBodyForTextAndFile) {
   scoped_refptr<network::ResourceRequestBody> body = ComputeMultipartBody(
       names, values, is_value_file_uris, filenames, types, boundary);
 
-  std::vector<network::mojom::DataElementType> expected_types = {
+  std::vector<network::DataElement::Tag> expected_types = {
       // item 1
-      network::mojom::DataElementType::kBytes,
+      network::DataElement::Tag::kBytes,
       // item 2
-      network::mojom::DataElementType::kBytes,
-      network::mojom::DataElementType::kFile,
-      network::mojom::DataElementType::kBytes,
+      network::DataElement::Tag::kBytes, network::DataElement::Tag::kFile,
+      network::DataElement::Tag::kBytes,
       // item 3
-      network::mojom::DataElementType::kBytes,
-      network::mojom::DataElementType::kFile,
-      network::mojom::DataElementType::kBytes,
+      network::DataElement::Tag::kBytes, network::DataElement::Tag::kFile,
+      network::DataElement::Tag::kBytes,
       // item 4
-      network::mojom::DataElementType::kBytes,
+      network::DataElement::Tag::kBytes,
       // item 5
-      network::mojom::DataElementType::kBytes,
-      network::mojom::DataElementType::kFile,
-      network::mojom::DataElementType::kBytes,
+      network::DataElement::Tag::kBytes, network::DataElement::Tag::kFile,
+      network::DataElement::Tag::kBytes,
       // item 6
-      network::mojom::DataElementType::kBytes,
+      network::DataElement::Tag::kBytes,
       // ending
-      network::mojom::DataElementType::kBytes};
+      network::DataElement::Tag::kBytes};
   std::vector<std::string> expected = {
       // item 1
       "--boundary\r\nContent-Disposition: form-data; "
@@ -185,9 +180,8 @@ TEST(TargetUtilTest, MultipartBodyWithPercentEncoding) {
       names, values, is_value_file_uris, filenames, types, boundary);
   EXPECT_NE(nullptr, body->elements());
 
-  std::vector<network::mojom::DataElementType> expected_types = {
-      network::mojom::DataElementType::kBytes,
-      network::mojom::DataElementType::kBytes};
+  std::vector<network::DataElement::Tag> expected_types = {
+      network::DataElement::Tag::kBytes, network::DataElement::Tag::kBytes};
   std::vector<std::string> expected = {
       "--boundary\r\nContent-Disposition: form-data;"
       " name=\"name\"; filename=\"filename\"\r\nContent-Type: type"

diff --git a/chrome/services/speech/cloud_speech_recognition_client_unittest.cc b/chrome/services/speech/cloud_speech_recognition_client_unittest.cc
@@ -184,16 +184,15 @@ void CloudSpeechRecognitionClientUnitTest::InitializeUpstreamPipeIfNecessary() {
           GetUpstreamRequest();
       EXPECT_TRUE(upstream_request);
       EXPECT_TRUE(upstream_request->request.request_body);
-      EXPECT_EQ(1u, upstream_request->request.request_body->elements()->size());
-      EXPECT_EQ(
-          network::mojom::DataElementType::kChunkedDataPipe,
-          (*upstream_request->request.request_body->elements())[0].type());
-      network::TestURLLoaderFactory::PendingRequest* mutable_upstream_request =
-          const_cast<network::TestURLLoaderFactory::PendingRequest*>(
-              upstream_request);
-      chunked_data_pipe_getter_.Bind((*mutable_upstream_request->request
-                                           .request_body->elements_mutable())[0]
-                                         .ReleaseChunkedDataPipeGetter());
+      auto& mutable_elements =
+          *upstream_request->request.request_body->elements_mutable();
+      ASSERT_EQ(1u, mutable_elements.size());
+      ASSERT_EQ(network::DataElement::Tag::kChunkedDataPipe,
+                mutable_elements[0].type());
+      chunked_data_pipe_getter_.Bind(
+          mutable_elements[0]
+              .As<network::DataElementChunkedDataPipe>()
+              .ReleaseChunkedDataPipeGetter());
     }
 
     constexpr size_t kDataPipeCapacity = 256;

diff --git a/components/autofill/core/browser/autofill_download_manager_unittest.cc b/components/autofill/core/browser/autofill_download_manager_unittest.cc
@@ -87,10 +87,11 @@ std::string GetStringFromDataElements(
     const std::vector<network::DataElement>* data_elements) {
   std::string result;
   for (const network::DataElement& e : *data_elements) {
-    DCHECK_EQ(e.type(), network::mojom::DataElementType::kBytes);
+    DCHECK_EQ(e.type(), network::DataElement::Tag::kBytes);
     // Provide the length of the bytes explicitly, not to rely on the null
     // termination.
-    result.append(e.bytes(), base::checked_cast<size_t>(e.length()));
+    const auto piece = e.As<network::DataElementBytes>().AsStringPiece();
+    result.append(piece.data(), piece.size());
   }
   return result;
 }

diff --git a/components/feed/core/v2/feed_network_impl_unittest.cc b/components/feed/core/v2/feed_network_impl_unittest.cc
@@ -476,7 +476,9 @@ TEST_F(FeedNetworkTest, SendActionRequestSendsValidRequest) {
   auto* elements = resource_request.request_body->elements();
   ASSERT_TRUE(elements);
   ASSERT_EQ(1UL, elements->size());
-  std::string sent_body((*elements)[0].bytes(), (*elements)[0].length());
+  ASSERT_EQ(network::DataElement::Tag::kBytes, elements->at(0).type());
+  std::string sent_body(
+      elements->at(0).As<network::DataElementBytes>().AsStringPiece());
   std::string sent_body_uncompressed;
   ASSERT_TRUE(compression::GzipUncompress(sent_body, &sent_body_uncompressed));
   std::string expected_body;

diff --git a/components/optimization_guide/core/hints_fetcher_unittest.cc b/components/optimization_guide/core/hints_fetcher_unittest.cc
@@ -139,7 +139,12 @@ class HintsFetcherTest : public testing::Test,
       EXPECT_EQ(pending_request.request.request_body->elements()->size(), 1u);
       auto& element =
           pending_request.request.request_body->elements_mutable()->front();
-      last_request_body_ = std::string(element.bytes(), element.length());
+      if (element.type() != network::DataElement::Tag::kBytes) {
+        ADD_FAILURE() << "network::DataElement type mismatch";
+        return;
+      }
+      last_request_body_ =
+          std::string(element.As<network::DataElementBytes>().AsStringPiece());
     }
   }
 

diff --git a/components/speech/upstream_loader.cc b/components/speech/upstream_loader.cc
@@ -19,8 +19,11 @@ UpstreamLoader::UpstreamLoader(
   // Attach a chunked upload body.
   mojo::PendingRemote<network::mojom::ChunkedDataPipeGetter> data_remote;
   receiver_set_.Add(this, data_remote.InitWithNewPipeAndPassReceiver());
-  resource_request->request_body = new network::ResourceRequestBody();
-  resource_request->request_body->SetToChunkedDataPipe(std::move(data_remote));
+  resource_request->request_body =
+      base::MakeRefCounted<network::ResourceRequestBody>();
+  resource_request->request_body->SetToChunkedDataPipe(
+      std::move(data_remote),
+      network::ResourceRequestBody::ReadOnlyOnce(false));
   simple_url_loader_ = network::SimpleURLLoader::Create(
       std::move(resource_request), upstream_traffic_annotation);
   simple_url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(

diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc
@@ -1286,20 +1286,20 @@ bool ChildProcessSecurityPolicyImpl::CanReadRequestBody(
 
   for (const network::DataElement& element : *body->elements()) {
     switch (element.type()) {
-      case network::mojom::DataElementType::kFile:
-        if (!CanReadFile(child_id, element.path()))
+      case network::DataElement::Tag::kFile:
+        if (!CanReadFile(child_id,
+                         element.As<network::DataElementFile>().path()))
           return false;
         break;
 
-      case network::mojom::DataElementType::kBytes:
+      case network::DataElement::Tag::kBytes:
         // Data is self-contained within |body| - no need to check access.
         break;
 
-      case network::mojom::DataElementType::kDataPipe:
+      case network::DataElement::Tag::kDataPipe:
         // Data is self-contained within |body| - no need to check access.
         break;
 
-      case network::mojom::DataElementType::kUnknown:
       default:
         // Fail safe - deny access.
         NOTREACHED();

diff --git a/content/browser/devtools/protocol/network_handler.cc b/content/browser/devtools/protocol/network_handler.cc
@@ -524,14 +524,15 @@ bool GetPostData(
     return false;
   for (const auto& element : *elements) {
     // TODO(caseq): Also support blobs.
-    if (element.type() != network::mojom::DataElementType::kBytes)
+    if (element.type() != network::DataElement::Tag::kBytes)
       return false;
-    auto bytes = protocol::Binary::fromSpan(
-        reinterpret_cast<const uint8_t*>(element.bytes()), element.length());
+    const std::vector<uint8_t>& bytes =
+        element.As<network::DataElementBytes>().bytes();
     auto data_entry = protocol::Network::PostDataEntry::Create().Build();
-    data_entry->SetBytes(std::move(bytes));
+    data_entry->SetBytes(
+        protocol::Binary::fromSpan(bytes.data(), bytes.size()));
     data_entries->push_back(std::move(data_entry));
-    result->append(element.bytes(), element.length());
+    result->append(reinterpret_cast<const char*>(bytes.data()), bytes.size());
   }
   return true;
 }

diff --git a/content/browser/service_worker/service_worker_main_resource_loader_unittest.cc b/content/browser/service_worker/service_worker_main_resource_loader_unittest.cc
@@ -197,8 +197,9 @@ class FetchEventServiceWorker : public FakeServiceWorker {
     // So far this test expects a single bytes element.
     ASSERT_EQ(1u, elements->size());
     const network::DataElement& element = elements->front();
-    ASSERT_EQ(network::mojom::DataElementType::kBytes, element.type());
-    *out_string = std::string(element.bytes(), element.length());
+    ASSERT_EQ(network::DataElement::Tag::kBytes, element.type());
+    *out_string =
+        std::string(element.As<network::DataElementBytes>().AsStringPiece());
   }
 
   void RunUntilFetchEvent() {

diff --git a/content/browser/speech/speech_recognition_engine_unittest.cc b/content/browser/speech/speech_recognition_engine_unittest.cc
@@ -709,15 +709,15 @@ std::string SpeechRecognitionEngineTest::ConsumeChunkedUploadData() {
       EXPECT_TRUE(upstream_request);
       EXPECT_TRUE(upstream_request->request.request_body);
       EXPECT_EQ(1u, upstream_request->request.request_body->elements()->size());
-      EXPECT_EQ(
-          network::mojom::DataElementType::kChunkedDataPipe,
-          (*upstream_request->request.request_body->elements())[0].type());
-      network::TestURLLoaderFactory::PendingRequest* mutable_upstream_request =
-          const_cast<network::TestURLLoaderFactory::PendingRequest*>(
-              upstream_request);
-      chunked_data_pipe_getter_.Bind((*mutable_upstream_request->request
-                                           .request_body->elements_mutable())[0]
-                                         .ReleaseChunkedDataPipeGetter());
+      auto& element =
+          (*upstream_request->request.request_body->elements_mutable())[0];
+      if (element.type() != network::DataElement::Tag::kChunkedDataPipe) {
+        ADD_FAILURE() << "element type mismatch";
+        return "";
+      }
+      chunked_data_pipe_getter_.Bind(
+          element.As<network::DataElementChunkedDataPipe>()
+              .ReleaseChunkedDataPipeGetter());
     }
     mojo::DataPipe data_pipe;
     chunked_data_pipe_getter_->StartReading(

diff --git a/content/browser/speech/speech_recognizer_impl_unittest.cc b/content/browser/speech/speech_recognizer_impl_unittest.cc
@@ -433,15 +433,12 @@ TEST_F(SpeechRecognizerImplTest, StopWithData) {
       ASSERT_TRUE(GetUpstreamRequest(&upstream_request));
       ASSERT_TRUE(upstream_request->request.request_body);
       ASSERT_EQ(1u, upstream_request->request.request_body->elements()->size());
-      ASSERT_EQ(
-          network::mojom::DataElementType::kChunkedDataPipe,
-          (*upstream_request->request.request_body->elements())[0].type());
-      network::TestURLLoaderFactory::PendingRequest* mutable_upstream_request =
-          const_cast<network::TestURLLoaderFactory::PendingRequest*>(
-              upstream_request);
-      chunked_data_pipe_getter.Bind((*mutable_upstream_request->request
-                                          .request_body->elements_mutable())[0]
-                                        .ReleaseChunkedDataPipeGetter());
+      auto& element =
+          (*upstream_request->request.request_body->elements_mutable())[0];
+      ASSERT_EQ(network::DataElement::Tag::kChunkedDataPipe, element.type());
+      chunked_data_pipe_getter.Bind(
+          element.As<network::DataElementChunkedDataPipe>()
+              .ReleaseChunkedDataPipeGetter());
       chunked_data_pipe_getter->StartReading(
           std::move(data_pipe.producer_handle));
     }

diff --git a/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc b/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
@@ -204,13 +204,14 @@ class FakeControllerServiceWorker
     // So far this test expects a single element (bytes or data pipe).
     ASSERT_EQ(1u, elements->size());
     network::DataElement& element = elements->front();
-    if (element.type() == network::mojom::DataElementType::kBytes) {
-      *out_string = std::string(element.bytes(), element.length());
-    } else if (element.type() == network::mojom::DataElementType::kDataPipe) {
+    if (element.type() == network::DataElement::Tag::kBytes) {
+      *out_string =
+          std::string(element.As<network::DataElementBytes>().AsStringPiece());
+    } else if (element.type() == network::DataElement::Tag::kDataPipe) {
       // Read the content into |data_pipe|.
       mojo::DataPipe data_pipe;
       mojo::Remote<network::mojom::DataPipeGetter> remote(
-          element.ReleaseDataPipeGetter());
+          element.As<network::DataElementDataPipe>().ReleaseDataPipeGetter());
       base::RunLoop run_loop;
       remote->Read(
           std::move(data_pipe.producer_handle),

diff --git a/extensions/browser/api/web_request/web_request_info.cc b/extensions/browser/api/web_request/web_request_info.cc
@@ -84,20 +84,19 @@ bool CreateUploadDataSourcesFromResourceRequest(
 
   for (auto& element : *request.request_body->elements()) {
     switch (element.type()) {
-      case network::mojom::DataElementType::kDataPipe:
+      case network::DataElement::Tag::kDataPipe:
         // TODO(https://crbug.com/721414): Support data pipe elements.
         break;
 
-      case network::mojom::DataElementType::kBytes:
+      case network::DataElement::Tag::kBytes:
         data_sources->push_back(std::make_unique<BytesUploadDataSource>(
-            base::StringPiece(element.bytes(), element.length())));
+            element.As<network::DataElementBytes>().AsStringPiece()));
         break;
-
-      case network::mojom::DataElementType::kFile:
+      case network::DataElement::Tag::kFile:
         // TODO(https://crbug.com/715679): This may not work when network
         // process is sandboxed.
-        data_sources->push_back(
-            std::make_unique<FileUploadDataSource>(element.path()));
+        data_sources->push_back(std::make_unique<FileUploadDataSource>(
+            element.As<network::DataElementFile>().path()));
         break;
 
       default:

diff --git a/google_apis/gaia/gaia_auth_fetcher_unittest.cc b/google_apis/gaia/gaia_auth_fetcher_unittest.cc
@@ -55,7 +55,10 @@ std::string GetRequestBodyAsString(const network::ResourceRequest* request) {
     return "";
   }
   const network::DataElement& elem = request->request_body->elements()->at(0);
-  return std::string(elem.bytes(), elem.length());
+  if (elem.type() != network::DataElement::Tag::kBytes) {
+    return "";
+  }
+  return std::string(elem.As<network::DataElementBytes>().AsStringPiece());
 }
 
 }  // namespace