win: Add implementation of HTTPTransport based on WinHTTP

util/net/http_transport_win.cc

+// Copyright 2015 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 <windows.h>
+#include <winhttp.h>
+
+#include "base/logging.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "util/net/http_body.h"
+
+namespace crashpad {
+
+namespace {
+
+// PLOG doesn't work for messages from WinHTTP, so we need to use
+// FORMAT_MESSAGE_FROM_HMODULE + the dll name manually here.
+void LogErrorWinHttpMessage(const char* extra) {
+  DWORD error_code = GetLastError();
+  const int kErrorMessageBufferSize = 256;

[cpu_(ooo_6.6-7.5), 2015/01/21 02:26:25]

nit: remove the constant name since it is not used anywhere.

[scottmg, 2015/01/21 19:48:27]

Done.

+  char msgbuf[kErrorMessageBufferSize];
+  DWORD flags = FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS |
+                FORMAT_MESSAGE_MAX_WIDTH_MASK | FORMAT_MESSAGE_FROM_HMODULE;
+  DWORD len = FormatMessageA(flags,
+                             GetModuleHandle(L"winhttp.dll"),
+                             error_code,
+                             0,
+                             msgbuf,
+                             arraysize(msgbuf),
+                             NULL);
+  if (len) {
+    LOG(ERROR) << extra << ": " << msgbuf
+               << base::StringPrintf(" (0x%X)", error_code);
+  } else {
+    LOG(ERROR) << base::StringPrintf(
+        "Error (0x%X) while retrieving error. (0x%X)",
+        GetLastError(),
+        error_code);
+  }
+}
+
+class HTTPTransportWin final : public HTTPTransport {
+ public:
+  HTTPTransportWin();
+  ~HTTPTransportWin() override;
+
+  bool ExecuteSynchronously() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(HTTPTransportWin);
+};
+
+HTTPTransportWin::HTTPTransportWin() : HTTPTransport() {
+}
+
+HTTPTransportWin::~HTTPTransportWin() {
+}
+
+bool HTTPTransportWin::ExecuteSynchronously() {
+  HINTERNET session = WinHttpOpen(L"Crashpad/1.0",

[cpu_(ooo_6.6-7.5), 2015/01/21 02:26:25]

magic string is magic. do we have the same string in mac?

[Mark Mentovai, 2015/01/21 19:23:17]

cpu wrote:
We don’t currently set the user-agent on Mac.

Rather than hard-coding this here, you can #include "package.h" and use
PACKAGE_NAME and PACKAGE_VERSION.

[scottmg, 2015/01/21 19:48:27]

Done.

+                                  WINHTTP_ACCESS_TYPE_DEFAULT_PROXY,
+                                  WINHTTP_NO_PROXY_NAME,
+                                  WINHTTP_NO_PROXY_BYPASS,

[cpu_(ooo_6.6-7.5), 2015/01/21 02:26:24]

we'll be fiddling with the proxy setting eventually, for now its fine.

[scottmg, 2015/01/21 19:48:27]

Acknowledged.

+                                  0);
+  if (!session) {
+    LogErrorWinHttpMessage("WinHttpOpen");
+    return false;
+  }
+
+  URL_COMPONENTS url_components = {0};
+  url_components.dwStructSize = sizeof(URL_COMPONENTS);
+  url_components.dwHostNameLength = 1;

[cpu_(ooo_6.6-7.5), 2015/01/21 02:26:24]

sample uses -1. I guess any value other than zero? Don't recall ever cracking
urls, sorry.

[scottmg, 2015/01/21 19:48:27]

Yeah, it's a bit crazy, explained here in "Remarks":
https://msdn.microsoft.com/en-us/library/windows/desktop/aa385420(v=vs.85).aspx 

I tried switching to -1, but then I get

d:\src\crashpad\crashpad\util\net\http_transport_win.cc(89) : warning C4245: '='
: conversion from 'int' to 'DWORD', signed/unsigned mismatch

so seems a bit uglier to cast -1, so just left it as 1.

+  url_components.dwUrlPathLength = 1;
+  url_components.dwExtraInfoLength = 1;
+  std::wstring url_wide(base::UTF8ToUTF16(url()));
+  if (!WinHttpCrackUrl(
+          url_wide.c_str(), 0, ICU_REJECT_USERPWD, &url_components)) {
+    LogErrorWinHttpMessage("WinHttpCrackUrl");
+    return false;
+  }
+  std::wstring host_name(url_components.lpszHostName,
+                         url_components.dwHostNameLength);
+  std::wstring url_path(url_components.lpszUrlPath,
+                        url_components.dwUrlPathLength);
+  std::wstring extra_info(url_components.lpszExtraInfo,
+                          url_components.dwExtraInfoLength);
+

[cpu_(ooo_6.6-7.5), 2015/01/21 02:26:25]

not using GURL in this project I assume?

[scottmg, 2015/01/21 19:48:27]

Not at the moment. Maybe we should?

+  HINTERNET connect =
+      WinHttpConnect(session, host_name.c_str(), url_components.nPort, 0);
+  if (!connect) {
+    LogErrorWinHttpMessage("WinHttpConnect");
+    return false;
+  }
+
+  HINTERNET request = WinHttpOpenRequest(connect,
+                                         base::UTF8ToUTF16(method()).c_str(),
+                                         url_path.c_str(),
+                                         nullptr,
+                                         WINHTTP_NO_REFERER,
+                                         WINHTTP_DEFAULT_ACCEPT_TYPES,
+                                         0);
+  if (!request) {
+    LogErrorWinHttpMessage("WinHttpOpenRequest");
+    return false;
+  }
+
+  // Add headers to the request.

[cpu_(ooo_6.6-7.5), 2015/01/21 02:26:24]

if the headers are trusted we are fine. Else we have issues.

[scottmg, 2015/01/21 19:48:27]

Good point, noted. At the moment it's only used in test code, but we'll have to
be careful in the future.

+  for (const auto& pair : headers()) {
+    std::wstring header_string =
+        base::UTF8ToUTF16(pair.first) + L": " + base::UTF8ToUTF16(pair.second);
+    if (!WinHttpAddRequestHeaders(request,
+                                  header_string.c_str(),
+                                  header_string.size(),
+                                  WINHTTP_ADDREQ_FLAG_ADD)) {
+      LogErrorWinHttpMessage("WinHttpAddRequestHeaders");
+      return false;
+    }
+  }
+
+  // We need the Content-Length up front, so buffer in memory. We should modify
+  // the interface to not require this, and then use WinHttpWriteData after
+  // WinHttpSendRequest.
+  std::vector<uint8_t> post_data;
+
+  // Write the body of a POST if any.
+  for (;;) {
+    uint8_t buffer[4 << 10];

[cpu_(ooo_6.6-7.5), 2015/01/21 02:26:24]

rather use the decimal (or hex constant) ? neither 4 or 10 are used in the
for().

[scottmg, 2015/01/21 19:48:27]

Done.

+    ssize_t bytes_to_write =
+        body_stream()->GetBytesBuffer(buffer, sizeof(buffer));
+    if (bytes_to_write == 0)
+      break;
+    post_data.insert(post_data.end(), buffer, buffer + bytes_to_write);

[Mark Mentovai, 2015/01/21 19:23:17]

Is there no callback-based way to get winhttp to ask for data? This will load
the entire dump into memory, which at some point may be quite large. That’s why
this was designed to consume a bunch of byte buffers, so that data could be
provided to the transport incrementally without having to buffer it all.

[scottmg, 2015/01/21 19:48:27]

It doesn't need the data up front (it can be written later as a stream with
WinHttpWriteData) but it does need the size for the Content-Length. I didn't see
a way to tell it to use Chunked, but perhaps that's also possible.

I planned to make a subsequent change to the to HttpBody interface to allow
determining the stream length ahead of time to avoid the extra copying.

[scottmg, 2015/01/21 20:43:59]

Hm, actually I have some reading comprehension problems. It's documented in
WinHttpSendRequest, but the built in support seems to be Vista+, so we may still
need to calculate Content-Length up front.

+  }
+
+  if (!WinHttpSendRequest(request,
+                          WINHTTP_NO_ADDITIONAL_HEADERS,
+                          0,
+                          &post_data[0],
+                          post_data.size(),
+                          post_data.size(),
+                          0)) {
+    LogErrorWinHttpMessage("WinHttpSendRequest");
+    return false;
+  }
+
+  if (!WinHttpReceiveResponse(request, nullptr)) {
+    LogErrorWinHttpMessage("WinHttpReceiveResponse");
+    return false;
+  }
+
+  DWORD status_code = 0;
+  DWORD sizeof_status_code = sizeof(status_code);
+
+  if (!WinHttpQueryHeaders(
+          request,
+          WINHTTP_QUERY_STATUS_CODE | WINHTTP_QUERY_FLAG_NUMBER,
+          WINHTTP_HEADER_NAME_BY_INDEX,
+          &status_code,
+          &sizeof_status_code,
+          WINHTTP_NO_HEADER_INDEX)) {
+    LogErrorWinHttpMessage("WinHttpQueryHeaders");
+    return false;
+  }
+
+  if (status_code != 200) {
+    LOG(ERROR) << base::StringPrintf("HTTP status %d", status_code);
+    return false;
+  }
+
+  // TODO(scottmg): Retrieve body of response if necessary with
+  // WinHttpQueryDataAvailable and WinHttpReadData.
+

[cpu_(ooo_6.6-7.5), 2015/01/21 02:26:25]

closing the handles only happens here below, not in a lot of error related early
returns.

I would not be oppose to a 'goto cleanup' or whatever. Some of the early returns
like in 157 can happen naturally so I fear that not closing will lead to
threadpool accumulations or other resource leaks.

[Mark Mentovai, 2015/01/21 19:23:17]

cpu wrote:
Sounds like a task for a simple ScopedGeneric-based scoper.

[scottmg, 2015/01/21 19:48:27]

Done.

+  if (!WinHttpCloseHandle(request)) {
+    LogErrorWinHttpMessage("WinHttpCloseHandle(request)");
+    return false;
+  }
+  if (!WinHttpCloseHandle(connect)) {
+    LogErrorWinHttpMessage("WinHttpCloseHandle(connect)");
+    return false;
+  }
+  if (!WinHttpCloseHandle(session)) {
+    LogErrorWinHttpMessage("WinHttpCloseHandle(session)");
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace
+
+// static
+scoped_ptr<HTTPTransport> HTTPTransport::Create() {
+  return scoped_ptr<HTTPTransportWin>(new HTTPTransportWin);
+}
+
+}  // namespace crashpad
+

[Mark Mentovai, 2015/01/21 19:23:17]

Blank line at EOF.

[scottmg, 2015/01/21 19:48:27]

Done.