Introducing a net::GenerateMimeMultipartBoundary helper.

google_apis/drive/base_requests.cc

Index: google_apis/drive/base_requests.cc
diff --git a/google_apis/drive/base_requests.cc b/google_apis/drive/base_requests.cc
index 3e5b0d1d4c75754b0ddc52eeed4fb6cd51e0aa94..359339ad8853d5b24aac35d6e7eb7802917344c8 100644
--- a/google_apis/drive/base_requests.cc
+++ b/google_apis/drive/base_requests.cc
@@ -27,6 +27,7 @@
 #include "net/base/elements_upload_data_stream.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
+#include "net/base/mime_util.h"
 #include "net/base/net_errors.h"
 #include "net/base/upload_bytes_element_reader.h"
 #include "net/base/upload_data_stream.h"
@@ -76,13 +77,6 @@ const char kMultipartItemHeaderFormat[] = "--%s\nContent-Type: %s\n\n";
 // Footer for whole multipart message.
 const char kMultipartFooterFormat[] = "--%s--";
 
-// Characters to be used for multipart/related boundary.
-const char kBoundaryCharacters[] =
-    "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
-
-// Size of multipart/related's boundary.
-const char kBoundarySize = 70;
-
 // Parses JSON passed in |json| on |blocking_task_runner|. Runs |callback| on
 // the calling thread when finished with either success or failure.
 // The callback must not be null.
@@ -226,24 +220,7 @@ void GenerateMultipartBody(MultipartType multipart_type,
   std::string boundary;
   // Generate random boundary.
   if (predetermined_boundary.empty()) {
-    while (true) {
-      boundary.resize(kBoundarySize);
-      for (int i = 0; i < kBoundarySize; ++i) {
-        // Subtract 2 from the array size to exclude '\0', and to turn the size
-        // into the last index.
-        const int last_char_index = arraysize(kBoundaryCharacters) - 2;
-        boundary[i] = kBoundaryCharacters[base::RandInt(0,  last_char_index)];
-      }
-      bool conflict_with_content = false;

[hashimoto, 2016/01/05 07:16:09]

The new implementation generates much less variety of boundary strings (62^70 is
2e77 times larger than 256^20).
Is it OK to remove this check?

BTW, why do we need to mimic IE/UnMHT when it results in only 256^20 possible
output strings?

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

The new implementation is generating 256^30 strings (30 random bytes in
random_values array which is equivalent to 40 base64 characters so 64^40).

I am open to increasing the entropy to 256^45 (by removing "MultipartBoundary--"
substring [to make room for additional 20 base64 characters] and therefore
slightly hurting readability of the generated mime documents), but I think that
shouldn't be necessary - I think that reintroducing the retries below should be
sufficient.
The probability of collisions is quite low (see the comment in [1] for more
details), but I can see that argument that 1) GDrive is potentially dealing with
encrypted files (i.e. effectively random content) and 2) GDrive is dealing with
long files (i.e. lots of potential collisions) and 3) this check-and-retry is
infinite (so it really is significantly changing the behavior, unlike the
at-most-3-retries-code in the other place).

FWIW, I've put the check-and-retry code back.

[1]
https://codereview.chromium.org/1547593002/diff/60001/chrome/browser/chromeos...
The boundary marker is not very similar to IE/UnMHT anymore.  OTOH, many
implementations used "-" characters as a prefix and suffix, so I want to retain
this (in case some third party consumers depend on this).  Additionally the
dashes and the "MultipartBoundary" substring help make the marker human-readable
(i.e. make reading the generated MHTML files easier for humans / developers).

[hashimoto, 2016/01/07 05:15:48]

lgtm
Speaking of compatibility, Blink's implementation doesn't add "----" suffix, and
the comment there says "However the following characters, though legal, cause
some sites to fail: (),./:=+".
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...)
Probably better to avoid using /=+ for boundary strings?

[Łukasz Anforowicz, 2016/01/07 19:04:13]

Thank you very much for pointing this out.  This does seem important and I
wasn't aware of this.  I guess this means I should back out of using
base::RandBytes + base::Base64Encode (rather than patch it up with
std::string::replace or something like this) and instead reuse code similar to
what was used in google_apis/drive/base_requests.cc (and in a few other places).

Done.

BTW: By removing the weird base64 size dependencies, this change also allows to
increase the entropy by increasing the characters count from 67 to 69.

-      for (auto& part : parts) {
-        if (part.data.find(boundary, 0) != std::string::npos) {
-          conflict_with_content = true;
-          break;
-        }
-      }
-      if (!conflict_with_content)
-        break;
-    }
+    boundary = net::GenerateMimeMultipartBoundary();
   } else {
     boundary = predetermined_boundary;
   }