Add HTTPTransport, a Mac implementation, and an end-to-end test.

util/net/http_transport_test.cc

Index: util/net/http_transport_test.cc
diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7d4550d7797b446d426f3c157513e4ce19a20c70
--- /dev/null
+++ b/util/net/http_transport_test.cc
@@ -0,0 +1,251 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/net/http_transport.h"
+
+#include <vector>
+
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/stringprintf.h"
+#include "build/build_config.h"
+#include "gtest/gtest.h"
+#include "util/file/fd_io.h"
+#include "util/net/http_body.h"
+#include "util/net/http_headers.h"
+#include "util/net/http_multipart_builder.h"
+#include "util/test/multiprocess_exec.h"
+
+#if defined(OS_MACOSX)
+#include "util/net/http_transport_mac.h"
+#endif
+
+namespace crashpad {
+namespace test {
+namespace {
+
+class HTTPTransportTestFixture : public MultiprocessExec {
+ public:
+  using RequestValidator =
+      void(*)(HTTPTransportTestFixture*, const std::string&);
+
+  HTTPTransportTestFixture(const HTTPHeaders& headers,
+                           scoped_ptr<HTTPBodyStream> body_stream,
+                           int http_response_code,
+                           RequestValidator request_validator)
+      : MultiprocessExec(),
+        headers_(headers),
+        body_stream_(body_stream.Pass()),
+        response_code_(http_response_code),
+        request_validator_(request_validator) {
+    // TODO(rsesek): Use a more robust mechanism to locate testdata
+    // <https://code.google.com/p/crashpad/issues/detail?id=4>.
+    SetChildCommand("util/net/http_transport_test_server.py", nullptr);
+  }
+
+  const HTTPHeaders& headers() { return headers_; }
+
+ private:
+  void MultiprocessParent() override {
+    // The child will write the HTTP server port number as a packed int to

[Mark Mentovai, 2014/10/30 21:13:55]

I’d prefer fixed-size (uint16_t). Fixed-size for response_code_ too.

[Robert Sesek, 2014/10/31 14:48:15]

uint16_t is not enough bits. Dynamic ports go all the way to
IPPORT_HILASTAUTO=65535. Used a uint32_t.

[Robert Sesek, 2014/10/31 15:12:00]

Sorry, I was holding Python's struct wrong. uint16_t works.

[Mark Mentovai, 2014/10/31 15:13:36]

Robert Sesek wrote:
I’m not sure why you think uint16_t isn’t wide enough to store 65535. Its range
is [0, 65535].

TCP (RFC 793) uses 16-bit fields for ports.

+    // stdout.
+    int port = -1;

[Mark Mentovai, 2014/10/30 21:13:55]

No initialization necessary. CheckedReadFD() is guaranteed to set port, or your
money back. No need to ASSERT on it, either.

[Robert Sesek, 2014/10/31 14:48:15]

Done.

+    CheckedReadFD(ReadPipeFD(), &port, sizeof(port));
+    ASSERT_NE(-1, port);
+
+    // Then the parent will write the test's HTTP response code.
+    CheckedWriteFD(WritePipeFD(), &response_code_, sizeof(response_code_));

[Mark Mentovai, 2014/10/30 21:13:55]

This wasn’t clear to me when I started reading through it, because I’m thinking
“the server is supposed to return the response code, what the hell is the client
writing it for?” Now I see that you’re giving the server’s desired return code
to it here, but you should add a comment saying that’s the protocol you’ve come
up with.

[Robert Sesek, 2014/10/31 14:48:15]

I thought the above comment covered this, but I adjusted it.

+
+    // Now execute the HTTP request.
+    scoped_ptr<HTTPTransport> transport(CreateTransport());
+    transport->SetMethod("POST");
+    transport->SetURL(base::StringPrintf("http://localhost:%d/upload", port));

[Mark Mentovai, 2014/10/30 21:13:55]

Since you listen explicitly on 127.0.0.1 in the Python script, you should use
that address here too (reducing any flake caused by local resolver stupidity).

[Robert Sesek, 2014/10/31 14:48:15]

Done.

+    for (const auto& pair : headers_) {
+      transport->SetHeader(pair.first, pair.second);
+    }
+    transport->SetBodyStream(body_stream_.Pass());
+
+    EXPECT_EQ(transport->ExecuteSynchronously(), (response_code_ == 200));
+
+    // Read until the child's stdout closes.
+    std::string request;
+    char buf[32];
+    ssize_t bytes_read;
+    while ((bytes_read = ReadFD(ReadPipeFD(), buf, sizeof(buf))) != 0) {
+      ASSERT_GE(bytes_read, 0);
+      request.append(buf, bytes_read);
+    }
+
+    if (request_validator_)
+      request_validator_(this, request);
+  }
+
+  scoped_ptr<HTTPTransport> CreateTransport() {

[Mark Mentovai, 2014/10/30 21:13:55]

Instead of doing this here, should we have a static Create() factory function on
HTTPTransport to centralize platform-specific creation?

[Robert Sesek, 2014/10/31 14:48:15]

I went back on forth for this, but since you brought it up, done.

+#if defined(OS_MACOSX)
+    return scoped_ptr<HTTPTransport>(new HTTPTransportMac());
+#else
+#error "No HTTP transport available."
+#endif
+  }
+
+  HTTPHeaders headers_;
+  scoped_ptr<HTTPBodyStream> body_stream_;
+  int response_code_;
+  RequestValidator request_validator_;
+};
+
+const char kMultipartFormData[] = "multipart/form-data";
+
+void GetHeaderField(const std::string& request,
+                    const std::string& header,
+                    std::string* value) {
+  size_t index = request.find(header);
+  ASSERT_NE(std::string::npos, index);
+  // Since the header is never the first line of the request, it should always
+  // be preceded by a CRLF.
+  EXPECT_EQ('\n', request[index - 1]);
+  EXPECT_EQ('\r', request[index - 2]);
+
+  index += header.length();
+  EXPECT_EQ(':', request[index++]);
+  // Per RFC 7230 §3.2, there can be one or more spaces or horizontal tabs.
+  // For testing purposes, just assume one space.
+  EXPECT_EQ(' ', request[index++]);
+
+  size_t header_end = request.find('\r', index);
+  ASSERT_NE(std::string::npos, header_end);
+
+  *value = request.substr(index, header_end - index);
+}
+
+void GetMultipartBoundary(const std::string& request,
+                          std::string* multipart_boundary) {
+  std::string content_type;
+  GetHeaderField(request, kContentType, &content_type);
+
+  ASSERT_GE(content_type.length(), strlen(kMultipartFormData));

[Mark Mentovai, 2014/10/30 21:13:55]

#include <string.h>

[Robert Sesek, 2014/10/31 14:48:15]

Done.

+  size_t index = strlen(kMultipartFormData);
+  EXPECT_EQ(kMultipartFormData, content_type.substr(0, index));
+
+  EXPECT_EQ(';', content_type[index++]);
+
+  size_t boundary_begin = content_type.find('=', index);
+  ASSERT_NE(std::string::npos, boundary_begin);
+  EXPECT_EQ('=', content_type[boundary_begin++]);
+  if (multipart_boundary) {
+    *multipart_boundary = content_type.substr(boundary_begin);
+  }
+}
+
+const char kBoundaryEq[] = "boundary=";
+
+void ValidFormData(HTTPTransportTestFixture* fixture,
+                   const std::string& request) {
+  std::string actual_boundary;
+  GetMultipartBoundary(request, &actual_boundary);
+
+  const auto& content_type = fixture->headers().find(kContentType);
+  ASSERT_NE(fixture->headers().end(), content_type);
+
+  size_t boundary = content_type->second.find(kBoundaryEq);
+  ASSERT_NE(std::string::npos, boundary);
+  std::string expected_boundary =
+      content_type->second.substr(boundary + strlen(kBoundaryEq));
+  EXPECT_EQ(expected_boundary, actual_boundary);
+
+  size_t body_start = request.find("\r\n\r\n");
+  ASSERT_NE(std::string::npos, body_start);
+  body_start += 4;
+
+  std::string expected = "--" + expected_boundary + "\r\n";
+  expected += "Content-Disposition: form-data; name=\"key1\"\r\n\r\n";
+  expected += "test\r\n";
+  ASSERT_LT(body_start + expected.length(), request.length());
+  EXPECT_EQ(expected, request.substr(body_start, expected.length()));
+
+  body_start += expected.length();
+
+  expected = "--" + expected_boundary + "\r\n";
+  expected += "Content-Disposition: form-data; name=\"key2\"\r\n\r\n";
+  expected += "--abcdefg123\r\n";
+  expected += "--" + expected_boundary + "--\r\n";
+  ASSERT_EQ(body_start + expected.length(), request.length());
+  EXPECT_EQ(expected, request.substr(body_start));
+}
+
+TEST(HTTPTransport, ValidFormData) {
+  HTTPMultipartBuilder builder;
+  builder.SetFormData("key1", "test");
+  builder.SetFormData("key2", "--abcdefg123");
+
+  HTTPHeaders headers;
+  headers.insert(builder.GetContentType());
+
+  HTTPTransportTestFixture test(headers, builder.GetBodyStream(), 200,
+      &ValidFormData);
+  test.Run();
+}
+
+const char kTextPlain[] = "text/plain";
+
+void ErrorResponse(HTTPTransportTestFixture* fixture,
+                   const std::string& request) {
+  std::string content_type;
+  GetHeaderField(request, kContentType, &content_type);
+  EXPECT_EQ(kTextPlain, content_type);
+}
+
+TEST(HTTPTransport, ErrorResponse) {
+  HTTPMultipartBuilder builder;
+  HTTPHeaders headers;
+  headers[kContentType] = kTextPlain;
+  HTTPTransportTestFixture test(headers, builder.GetBodyStream(),
+      404, &ErrorResponse);
+  test.Run();
+}
+
+const char kTextBody[] = "hello world";
+
+void UnchunkedPlainText(HTTPTransportTestFixture* fixture,
+                        const std::string& request) {
+  std::string header_value;
+  GetHeaderField(request, kContentType, &header_value);
+  EXPECT_EQ(kTextPlain, header_value);
+
+  GetHeaderField(request, kContentLength, &header_value);
+  const auto& content_length = fixture->headers().find(kContentLength);
+  ASSERT_NE(fixture->headers().end(), content_length);
+  EXPECT_EQ(content_length->second, header_value);
+
+  size_t body_start = request.rfind("\r\n");
+  ASSERT_NE(std::string::npos, body_start);
+
+  EXPECT_EQ(kTextBody, request.substr(body_start + 2));
+}
+
+TEST(HTTPTransport, UnchunkedPlainText) {
+  scoped_ptr<HTTPBodyStream> body_stream(new StringHTTPBodyStream(kTextBody));
+
+  HTTPHeaders headers;
+  headers[kContentType] = kTextPlain;
+  headers[kContentLength] = base::StringPrintf("%zu", strlen(kTextBody));
+
+  HTTPTransportTestFixture test(headers, body_stream.Pass(), 200,
+      &UnchunkedPlainText);
+  test.Run();
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad

[Mark Mentovai, 2014/10/30 21:13:55]

Nothing with a file part? Are you just trusting that HTTPMultipartBuilder did
its job?

[Robert Sesek, 2014/10/31 14:48:15]

Yes, we should rely on composition of components and trust that they do what
they say.