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 <utility>
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/rand_util.h"

[bartfab (slow), 2016/01/05 14:20:45]

Nit: No longer used.

[Łukasz Anforowicz, 2016/01/05 19:16:48]

Done.

 #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;

[bartfab (slow), 2016/01/05 14:20:45]

Nit: #include <stddef.h>

[Łukasz Anforowicz, 2016/01/05 19:16:48]

Done.

 
 }  // namespace
 
 UploadJobImpl::Delegate::~Delegate() {
 }
 
 UploadJobImpl::MimeBoundaryGenerator::~MimeBoundaryGenerator() {
 }
 
 UploadJobImpl::RandomMimeBoundaryGenerator::~RandomMimeBoundaryGenerator() {
@@ skipping 22 matching lines @@
   const std::string& GetFilename() const;
 
   // Returns the data contained in this DataSegment. Ownership is passed.
   scoped_ptr<std::string> GetData();
 
   // Returns the size in bytes of the blob in |data_|.
   size_t GetDataSize() const;
 
   // Helper method that performs a substring match of |chunk| in |data_|.
   // Returns |true| if |chunk| matches a substring, |false| otherwise.
   bool CheckIfDataContains(const std::string& chunk);

[bartfab (slow), 2016/01/05 14:20:45]

Nit: This method is no longer used.

[Łukasz Anforowicz, 2016/01/05 19:16:48]

Done.  Thanks for catching this and other places where I've left code that could
be removed further.  I'll wear a cone of shame for a day... :-/

 
  private:
   const std::string name_;
   const std::string filename_;
   scoped_ptr<std::string> data_;
   std::map<std::string, std::string> header_entries_;
 
   DISALLOW_COPY_AND_ASSIGN(DataSegment);
 };
 
@@ skipping 29 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 57 matching lines @@
     return true;
 
   std::set<std::string> used_names;
 
   // Check uniqueness of header field names.
   for (const auto& data_segment : data_segments_) {
     if (!used_names.insert(data_segment->GetName()).second)
       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)));
-    for (const auto& data_segment : data_segments_) {
-      if (data_segment->CheckIfDataContains(*mime_boundary_)) {
-        found = false;
-        break;
-      }
-    }
-    ++retry;
-  } while (!found && retry <= kMimeBoundaryRetries);
-
-  // Notify the delegate that content encoding failed.
-  if (!found) {
-    delegate_->OnFailure(CONTENT_ENCODING_ERROR);

[bartfab (slow), 2016/01/05 14:20:45]

Nit: Remove CONTENT_ENCODING_ERROR from upload_job.h. It is no longer used.

[Łukasz Anforowicz, 2016/01/05 19:16:48]

Done.

-    mime_boundary_.reset();
-    return false;
-  }
+  mime_boundary_.reset(
+      new std::string(boundary_generator_->GenerateBoundary()));
 
   // 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