From f496130fd594a0467d7dacdf923a9677cafe280b Mon Sep 17 00:00:00 2001
From: Mark Mentovai <mark@chromium.org>
Date: Tue, 18 Aug 2015 15:42:34 -0400
Subject: [PATCH] HTTPTransportMac: CFStream Read() must always set at_eof
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CFStream’s CFReadStreamGetBuffer() calls the Read() callback without
initializing at_eof. The callback function is responsible for setting it
on any successful read operation. See 10.10.2 CF-1152.14/CFStream.c.

By chance, at_eof seems to always have an initial value of false on
x86_64, but true on 32-bit x86. Crashpad’s Read() callback assumed that
the initial value was always false. The discrepancy caused truncation
and possibly hangs when a 32-bit process attempted to upload a request
body larger than 32kB, the buffer size used by NSMutableURLRequest or
something between it and CFReadStream.

A new test with more than 32kB of data is added.

As discussed in:
https://groups.google.com/a/chromium.org/d/topic/crashpad-dev/Vz--qMZJRPU

TEST=crashpad_util_test HTTPTransport.Upload33k
BUG=
R=rsesek@chromium.org

Review URL: https://codereview.chromium.org/1304433004 .
---
 util/net/http_transport_mac.mm  | 10 ++++++----
 util/net/http_transport_test.cc | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/util/net/http_transport_mac.mm b/util/net/http_transport_mac.mm
index 9688d18c..c0ff7d18 100644
--- a/util/net/http_transport_mac.mm
+++ b/util/net/http_transport_mac.mm
@@ -87,15 +87,17 @@ class HTTPBodyStreamCFReadStream {
                       CFStreamError* error,
                       Boolean* at_eof,
                       void* info) {
-    if (buffer_length == 0)
+    if (buffer_length == 0) {
+      *at_eof = FALSE;
       return 0;
+    }
 
     ssize_t bytes_read = GetStream(info)->GetBytesBuffer(buffer, buffer_length);
-    if (bytes_read == 0) {
-      *at_eof = TRUE;
-    } else if (bytes_read < 0) {
+    if (bytes_read < 0) {
       error->error = -1;
       error->domain = kCFStreamErrorDomainCustom;
+    } else {
+      *at_eof = bytes_read == 0;
     }
 
     return bytes_read;
diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc
index dc3775b1..25a1a43a 100644
--- a/util/net/http_transport_test.cc
+++ b/util/net/http_transport_test.cc
@@ -278,6 +278,28 @@ TEST(HTTPTransport, UnchunkedPlainText) {
   test.Run();
 }
 
+TEST(HTTPTransport, Upload33k) {
+  // On OS X, NSMutableURLRequest winds up calling into a CFReadStream’s Read()
+  // callback with a 32kB buffer. Make sure that it’s able to get everything
+  // when enough is available to fill this buffer, requiring more than one
+  // Read().
+
+  std::string request_string(33 * 1024, 'a');
+  scoped_ptr<HTTPBodyStream> body_stream(
+      new StringHTTPBodyStream(request_string));
+
+  HTTPHeaders headers;
+  headers[kContentType] = "application/octet-stream";
+  headers[kContentLength] =
+      base::StringPrintf("%" PRIuS, request_string.size());
+  HTTPTransportTestFixture test(headers, body_stream.Pass(), 200,
+      [](HTTPTransportTestFixture* fixture, const std::string& request) {
+        size_t body_start = request.rfind("\r\n");
+        EXPECT_EQ(33 * 1024u + 2, request.size() - body_start);
+      });
+  test.Run();
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace crashpad