Introducing a net::GenerateMimeMultipartBoundary helper.

chrome/browser/chromeos/policy/upload_job_impl.cc

 // Copyright (c) 2015 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 "chrome/browser/chromeos/policy/upload_job_impl.h"
 
 #include <set>
 
 #include "base/logging.h"
-#include "base/rand_util.h"
 #include "base/strings/stringprintf.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/google_service_auth_error.h"
+#include "net/base/mime_util.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_request_status.h"
 
 namespace policy {
 
 namespace {
 
-// Defines the characters that might appear in strings generated by
-// GenerateRandomString().
-const char kAlphaNum[] =
-    "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
-
 // Format for bearer tokens in HTTP requests to access OAuth 2.0 protected
 // resources.
 const char kAuthorizationHeaderFormat[] = "Authorization: Bearer %s";
 
-// Prefix added to a randomly generated string when choosing the MIME boundary.
-const char kMultipartBoundaryPrefix[] = "----**--";
-
-// Postfix added to a randomly generated string when choosing the MIME boundary.
-const char kMultipartBoundaryPostfix[] = "--**----";
-
 // Value the "Content-Type" field will be set to in the POST request.
 const char kUploadContentType[] = "multipart/form-data";
 
 // Number of retries when randomly generating a MIME boundary.
 const int kMimeBoundaryRetries = 3;
 
-// Length of the random string for the MIME boundary.
-const int kMimeBoundarySize = 32;
-
 // Number of upload retries.
 const int kMaxRetries = 1;
 
-// Generates a random alphanumeric string of length |length|.
-std::string GenerateRandomString(size_t length) {
-  std::string random;
-  random.reserve(length);
-  for (size_t i = 0; i < length; i++)
-    random.push_back(kAlphaNum[base::RandGenerator(sizeof(kAlphaNum) - 1)]);
-  return random;
-}
+// Max size of MIME boundary according to RFC 1341, section 7.2.1.
+const size_t kMaxMimeBoundarySize = 70;
 
 }  // namespace
 
 UploadJobImpl::Delegate::~Delegate() {
 }
 
 UploadJobImpl::MimeBoundaryGenerator::~MimeBoundaryGenerator() {
 }
 
 UploadJobImpl::RandomMimeBoundaryGenerator::~RandomMimeBoundaryGenerator() {
@@ skipping 72 matching lines @@
 bool DataSegment::CheckIfDataContains(const std::string& chunk) {
   DCHECK(data_);
   return data_->find(chunk) != std::string::npos;
 }
 
 size_t DataSegment::GetDataSize() const {
   DCHECK(data_);
   return data_->size();
 }
 
-std::string UploadJobImpl::RandomMimeBoundaryGenerator::GenerateBoundary(
-    size_t length) const {
-  std::string boundary;
-  boundary.reserve(length);
-  DCHECK_GT(length, sizeof(kMultipartBoundaryPrefix) +
-                        sizeof(kMultipartBoundaryPostfix));
-  const size_t random_part_length = length - sizeof(kMultipartBoundaryPrefix) -
-                                    sizeof(kMultipartBoundaryPostfix);
-  boundary.append(kMultipartBoundaryPrefix);
-  boundary.append(GenerateRandomString(random_part_length));
-  boundary.append(kMultipartBoundaryPostfix);
-  return boundary;
+std::string UploadJobImpl::RandomMimeBoundaryGenerator::GenerateBoundary()
+    const {
+  return net::GenerateMimeMultipartBoundary();
 }
 
 UploadJobImpl::UploadJobImpl(
     const GURL& upload_url,
     const std::string& account_id,
     OAuth2TokenService* token_service,
     scoped_refptr<net::URLRequestContextGetter> url_context_getter,
     Delegate* delegate,
     scoped_ptr<MimeBoundaryGenerator> boundary_generator)
     : OAuth2TokenService::Consumer("cros_upload_job"),
@@ skipping 64 matching lines @@
       return false;
   }
 
   // Generates random MIME boundaries and tests if they appear in any of the
   // data segments. Tries up to |kMimeBoundaryRetries| times to find a MIME
   // boundary that does not appear within any data segment.
   bool found = false;
   int retry = 0;
   do {
     found = true;
-    mime_boundary_.reset(new std::string(
-        boundary_generator_->GenerateBoundary(kMimeBoundarySize)));
+    mime_boundary_.reset(
+        new std::string(boundary_generator_->GenerateBoundary()));
     for (const auto& data_segment : data_segments_) {
       if (data_segment->CheckIfDataContains(*mime_boundary_)) {
         found = false;
         break;
       }
     }
     ++retry;
   } while (!found && retry <= kMimeBoundaryRetries);

[Ryan Sleevi, 2015/12/28 23:30:00]

This seems overly conservative/unnecessary with your new change

[Łukasz Anforowicz, 2015/12/29 18:53:19]

Done.

I think you have a good point wrt removing the checks of whether the mime
boundary appears in the parts themselves.

1. The checks do seem unnecessary:

   1.a. The checks are only done by a subset of callsites
   1.b. The new code has plenty of entropy:
        - new code = 8 bits * 30 = 240 bits of entropy
        - old upload_job_impl.cc = 6 bits * (32-8-8) = 96 bits of entropy
        - old base_requests.cc = 6 bits * 70 = 420 bits of entropy
        - random guid (Variant 2, Version 4 RFC 4122 GUID) = 122 bits of
entropy.  It would take about 3.27e17 random GUIDs for a 1% chance of collision.
That’s more than 326 million billion. That’s a lot. [1]

2. Removing the checks is beneficial:

   2.a. Removal will simplify the code (which is good for readability +
maintainability and good for binary size).

   2.b. Avoiding the runtime overhead of the checks can potentially yield small
improvement in performance.


[1] http://blog.stephencleary.com/2010/11/few-words-on-guids.html

 
   // Notify the delegate that content encoding failed.
   if (!found) {
     delegate_->OnFailure(CONTENT_ENCODING_ERROR);
     mime_boundary_.reset();
     return false;
   }
 
   // Estimate an upper bound for the total message size to make memory
   // allocation more efficient. It is not an error if this turns out to be too
   // small as std::string will take care of the realloc.
   size_t size = 0;
   for (const auto& data_segment : data_segments_) {
     for (const auto& entry : data_segment->GetHeaderEntries())
       size += entry.first.size() + entry.second.size();
-    size += kMimeBoundarySize + data_segment->GetName().size() +
+    size += kMaxMimeBoundarySize + data_segment->GetName().size() +
             data_segment->GetFilename().size() + data_segment->GetDataSize();
     // Add some extra space for all the constants and control characters.
     size += 128;
   }
 
   // Allocate memory of the expected size.
   post_data_.reset(new std::string);
   post_data_->reserve(size);
 
   for (const auto& data_segment : data_segments_) {
@@ skipping 122 matching lines @@
   if (success) {
     state_ = SUCCESS;
     delegate_->OnSuccess();
   } else {
     state_ = ERROR;
     delegate_->OnFailure(SERVER_ERROR);
   }
 }
 
 }  // namespace policy