Use a FileReaderInterface for file attachments instead of a FilePath

This is a step towards a database which gives out FileReaders in Report
objects instead of FilePaths.

Change-Id: I59704da65fc5521e5d47019416bf962c215d13bc
Reviewed-on: https://chromium-review.googlesource.com/721978
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

handler/crash_report_upload_thread.cc

@@ -70,7 +70,7 @@ void InsertOrReplaceMapEntry(std::map<std::string, std::string>* map,
 //
 // In the event of an error reading the minidump file, a message will be logged.
 std::map<std::string, std::string> BreakpadHTTPFormParametersFromMinidump(
-    FileReader* minidump_file_reader) {
+    FileReaderInterface* minidump_file_reader) {
   ProcessSnapshotMinidump minidump_process_snapshot;
   if (!minidump_process_snapshot.Initialize(minidump_file_reader)) {
     return std::map<std::string, std::string>();
@@ -344,18 +344,25 @@ CrashReportUploadThread::UploadResult CrashReportUploadThread::UploadReport(
     std::string* response_body) {
   std::map<std::string, std::string> parameters;
 
-  {
+  FileReader minidump_file_reader;
-    FileReader minidump_file_reader;
+  if (!minidump_file_reader.Open(report->file_path)) {
-    if (!minidump_file_reader.Open(report->file_path)) {
+    // If the minidump file can’t be opened, all hope is lost.
-      // If the minidump file can’t be opened, all hope is lost.
+    return UploadResult::kPermanentFailure;
-      return UploadResult::kPermanentFailure;
+  }
-    }
 
-    // If the minidump file could be opened, ignore any errors that might occur
+  FileOffset start_offset = minidump_file_reader.SeekGet();
-    // when attempting to interpret it. This may result in its being uploaded
+  if (start_offset < 0) {
-    // with few or no parameters, but as long as there’s a dump file, the server
+    return UploadResult::kPermanentFailure;
-    // can decide what to do with it.
+  }
-    parameters = BreakpadHTTPFormParametersFromMinidump(&minidump_file_reader);
+
+  // Ignore any errors that might occur when attempting to interpret the
+  // minidump file. This may result in its being uploaded with few or no
+  // parameters, but as long as there’s a dump file, the server can decide what
+  // to do with it.
+  parameters = BreakpadHTTPFormParametersFromMinidump(&minidump_file_reader);
+
+  if (!minidump_file_reader.SeekSet(start_offset)) {
+    return UploadResult::kPermanentFailure;
   }
 
   HTTPMultipartBuilder http_multipart_builder;
@@ -379,7 +386,7 @@ CrashReportUploadThread::UploadResult CrashReportUploadThread::UploadReport(
 #else
       report->file_path.BaseName().value(),
 #endif
-      report->file_path,
+      &minidump_file_reader,
       "application/octet-stream");
 
   std::unique_ptr<HTTPTransport> http_transport(HTTPTransport::Create());

tools/crashpad_http_upload.cc

@@ -19,9 +19,11 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/files/file_path.h"
 #include "tools/tool_support.h"
+#include "util/file/file_reader.h"
 #include "util/file/file_writer.h"
 #include "util/net/http_body.h"
 #include "util/net/http_multipart_builder.h"
@@ -85,6 +87,7 @@ int HTTPUploadMain(int argc, char* argv[]) {
       {nullptr, 0, nullptr, 0},
   };
 
+  std::vector<std::unique_ptr<FileReader>> readers;
   HTTPMultipartBuilder http_multipart_builder;
 
   int opt;
@@ -102,8 +105,14 @@ int HTTPUploadMain(int argc, char* argv[]) {
             ToolSupport::CommandLineArgumentToFilePathStringType(path));
         std::string file_name(
             ToolSupport::FilePathToCommandLineArgument(file_path.BaseName()));
+
+        readers.push_back(std::make_unique<FileReader>());
+        FileReader* upload_file_reader = readers.back().get();
+        if (!upload_file_reader->Open(file_path)) {
+          return EXIT_FAILURE;
+        }
         http_multipart_builder.SetFileAttachment(
-            key, file_name, file_path, "application/octet-stream");
+            key, file_name, upload_file_reader, "application/octet-stream");
         break;
       }
       case kOptionNoUploadGzip: {

util/net/http_body.cc

@@ -46,38 +46,22 @@ FileOperationResult StringHTTPBodyStream::GetBytesBuffer(uint8_t* buffer,
   return num_bytes_returned;
 }
 
-FileHTTPBodyStream::FileHTTPBodyStream(const base::FilePath& path)
+FileReaderHTTPBodyStream::FileReaderHTTPBodyStream(FileReaderInterface* reader)
-    : HTTPBodyStream(), path_(path), file_(), file_state_(kUnopenedFile) {
+    : HTTPBodyStream(), reader_(reader), reached_eof_(false) {
+  DCHECK(reader_);
 }
 
-FileHTTPBodyStream::~FileHTTPBodyStream() {
+FileReaderHTTPBodyStream::~FileReaderHTTPBodyStream() {}
-}
 
-FileOperationResult FileHTTPBodyStream::GetBytesBuffer(uint8_t* buffer,
+FileOperationResult FileReaderHTTPBodyStream::GetBytesBuffer(uint8_t* buffer,
-                                                       size_t max_len) {
+                                                             size_t max_len) {
-  switch (file_state_) {
+  if (reached_eof_) {
-    case kUnopenedFile:
+    return 0;
-      file_.reset(LoggingOpenFileForRead(path_));
-      if (!file_.is_valid()) {
-        file_state_ = kFileOpenError;
-        return -1;
-      }
-      file_state_ = kReading;
-      break;
-    case kFileOpenError:
-      return -1;
-    case kClosedAtEOF:
-      return 0;
-    case kReading:
-      break;
   }
 
-  FileOperationResult rv = ReadFile(file_.get(), buffer, max_len);
+  FileOperationResult rv = reader_->Read(buffer, max_len);
   if (rv == 0) {
-    file_.reset();
+    reached_eof_ = true;
-    file_state_ = kClosedAtEOF;
-  } else if (rv < 0) {
-    PLOG(ERROR) << "read";
   }
   return rv;
 }

util/net/http_body.h

@@ -24,6 +24,7 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "util/file/file_io.h"
+#include "util/file/file_reader.h"
 
 namespace crashpad {
 
@@ -70,33 +71,26 @@ class StringHTTPBodyStream : public HTTPBodyStream {
   DISALLOW_COPY_AND_ASSIGN(StringHTTPBodyStream);
 };
 
-//! \brief An implementation of HTTPBodyStream that reads from the specified
+//! \brief An implementation of HTTPBodyStream that reads from a
-//!     file and provides its contents for an HTTP body.
+//!     FileReaderInterface and provides its contents for an HTTP body.
-class FileHTTPBodyStream : public HTTPBodyStream {
+class FileReaderHTTPBodyStream : public HTTPBodyStream {
  public:
-  //! \brief Creates a stream for reading the file at the specified \a path.
+  //! \brief Creates a stream for reading from a FileReaderInterface.
   //!
-  //! \param[in] path The file from which this HTTPBodyStream will read.
+  //! \param[in] reader A FileReaderInterface from which this HTTPBodyStream
-  explicit FileHTTPBodyStream(const base::FilePath& path);
+  //!     will read.
+  explicit FileReaderHTTPBodyStream(FileReaderInterface* reader);
 
-  ~FileHTTPBodyStream() override;
+  ~FileReaderHTTPBodyStream() override;
 
   // HTTPBodyStream:
   FileOperationResult GetBytesBuffer(uint8_t* buffer, size_t max_len) override;
 
  private:
-  enum FileState {
+  FileReaderInterface* reader_;  // weak
-    kUnopenedFile,
+  bool reached_eof_;
-    kFileOpenError,
-    kClosedAtEOF,
-    kReading,
-  };
 
-  base::FilePath path_;
+  DISALLOW_COPY_AND_ASSIGN(FileReaderHTTPBodyStream);
-  ScopedFileHandle file_;
-  FileState file_state_;
-
-  DISALLOW_COPY_AND_ASSIGN(FileHTTPBodyStream);
 };
 
 //! \brief An implementation of HTTPBodyStream that combines an array of

util/net/http_body_test.cc

@@ -96,10 +96,13 @@ TEST(StringHTTPBodyStream, MultipleReads) {
   }
 }
 
-TEST(FileHTTPBodyStream, ReadASCIIFile) {
+TEST(FileReaderHTTPBodyStream, ReadASCIIFile) {
   base::FilePath path = TestPaths::TestDataRoot().Append(
       FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt"));
-  FileHTTPBodyStream stream(path);
+
+  FileReader reader;
+  ASSERT_TRUE(reader.Open(path));
+  FileReaderHTTPBodyStream stream(&reader);
   std::string contents = ReadStreamToString(&stream, 32);
   EXPECT_EQ(contents, "This is a test.\n");
 
@@ -113,14 +116,16 @@ TEST(FileHTTPBodyStream, ReadASCIIFile) {
   ExpectBufferSet(buf, '!', sizeof(buf));
 }
 
-TEST(FileHTTPBodyStream, ReadBinaryFile) {
+TEST(FileReaderHTTPBodyStream, ReadBinaryFile) {
   // HEX contents of file: |FEEDFACE A11A15|.
   base::FilePath path = TestPaths::TestDataRoot().Append(
       FILE_PATH_LITERAL("util/net/testdata/binary_http_body.dat"));
   // This buffer size was chosen so that reading the file takes multiple reads.
   uint8_t buf[4];
 
-  FileHTTPBodyStream stream(path);
+  FileReader reader;
+  ASSERT_TRUE(reader.Open(path));
+  FileReaderHTTPBodyStream stream(&reader);
 
   memset(buf, '!', sizeof(buf));
   EXPECT_EQ(stream.GetBytesBuffer(buf, sizeof(buf)), 4);
@@ -143,18 +148,6 @@ TEST(FileHTTPBodyStream, ReadBinaryFile) {
   ExpectBufferSet(buf, '!', sizeof(buf));
 }
 
-TEST(FileHTTPBodyStream, NonExistentFile) {
-  base::FilePath path = base::FilePath(
-      FILE_PATH_LITERAL("/var/empty/crashpad/util/net/http_body/null"));
-  FileHTTPBodyStream stream(path);
-
-  uint8_t buf = 0xff;
-  EXPECT_LT(stream.GetBytesBuffer(&buf, 1), 0);
-  EXPECT_EQ(buf, 0xff);
-  EXPECT_LT(stream.GetBytesBuffer(&buf, 1), 0);
-  EXPECT_EQ(buf, 0xff);
-}
-
 TEST(CompositeHTTPBodyStream, TwoEmptyStrings) {
   std::vector<HTTPBodyStream*> parts;
   parts.push_back(new StringHTTPBodyStream(std::string()));
@@ -201,7 +194,10 @@ TEST_P(CompositeHTTPBodyStreamBufferSize, StringsAndFile) {
   parts.push_back(new StringHTTPBodyStream(string1));
   base::FilePath path = TestPaths::TestDataRoot().Append(
       FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt"));
-  parts.push_back(new FileHTTPBodyStream(path));
+
+  FileReader reader;
+  ASSERT_TRUE(reader.Open(path));
+  parts.push_back(new FileReaderHTTPBodyStream(&reader));
   parts.push_back(new StringHTTPBodyStream(string2));
 
   CompositeHTTPBodyStream stream(parts);

util/net/http_multipart_builder.cc

@@ -133,13 +133,13 @@ void HTTPMultipartBuilder::SetFormData(const std::string& key,
 void HTTPMultipartBuilder::SetFileAttachment(
     const std::string& key,
     const std::string& upload_file_name,
-    const base::FilePath& path,
+    FileReaderInterface* reader,
     const std::string& content_type) {
   EraseKey(upload_file_name);
 
   FileAttachment attachment;
   attachment.filename = EncodeMIMEField(upload_file_name);
-  attachment.path = path;
+  attachment.reader = reader;
 
   if (content_type.empty()) {
     attachment.content_type = "application/octet-stream";
@@ -174,7 +174,7 @@ std::unique_ptr<HTTPBodyStream> HTTPMultipartBuilder::GetBodyStream() {
         attachment.content_type.c_str(), kBoundaryCRLF);
 
     streams.push_back(new StringHTTPBodyStream(header));
-    streams.push_back(new FileHTTPBodyStream(attachment.path));
+    streams.push_back(new FileReaderHTTPBodyStream(attachment.reader));
     streams.push_back(new StringHTTPBodyStream(kCRLF));
   }
 

util/net/http_multipart_builder.h

@@ -19,8 +19,8 @@
 #include <memory>
 #include <string>
 
-#include "base/files/file_path.h"
 #include "base/macros.h"
+#include "util/file/file_reader.h"
 #include "util/net/http_headers.h"
 
 namespace crashpad {
@@ -51,7 +51,7 @@ class HTTPMultipartBuilder {
   //! \param[in] value The value to set at the \a key.
   void SetFormData(const std::string& key, const std::string& value);
 
-  //! \brief Specifies the file at \a path to have its contents uploaded as
+  //! \brief Specifies the contents read from \a reader to be uploaded as
   //!     multipart data, available at `name` of \a upload_file_name.
   //!
   //! \param[in] key The key of the form data, specified as the `name` in the
@@ -59,12 +59,13 @@ class HTTPMultipartBuilder {
   //!     key will be overwritten.
   //! \param[in] upload_file_name The `filename` to specify for this multipart
   //!     data attachment.
-  //! \param[in] path The path of the file whose contents will be uploaded.
+  //! \param[in] reader A FileReaderInterface from which to read the content to
+  //!     upload.
   //! \param[in] content_type The `Content-Type` to specify for the attachment.
   //!     If this is empty, `"application/octet-stream"` will be used.
   void SetFileAttachment(const std::string& key,
                          const std::string& upload_file_name,
-                         const base::FilePath& path,
+                         FileReaderInterface* reader,
                          const std::string& content_type);
 
   //! \brief Generates the HTTPBodyStream for the data currently supplied to
@@ -83,7 +84,7 @@ class HTTPMultipartBuilder {
   struct FileAttachment {
     std::string filename;
     std::string content_type;
-    base::FilePath path;
+    FileReaderInterface* reader;
   };
 
   // Removes elements from both data maps at the specified |key|, to ensure

util/net/http_multipart_builder_test.cc

@@ -102,18 +102,19 @@ TEST(HTTPMultipartBuilder, ThreeFileAttachments) {
   HTTPMultipartBuilder builder;
   base::FilePath ascii_http_body_path = TestPaths::TestDataRoot().Append(
       FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt"));
-  builder.SetFileAttachment("first",
+
-                            "minidump.dmp",
+  FileReader reader1;
-                            ascii_http_body_path,
+  ASSERT_TRUE(reader1.Open(ascii_http_body_path));
-                            "");
+  builder.SetFileAttachment("first", "minidump.dmp", &reader1, "");
-  builder.SetFileAttachment("second",
+
-                            "minidump.dmp",
+  FileReader reader2;
-                            ascii_http_body_path,
+  ASSERT_TRUE(reader2.Open(ascii_http_body_path));
-                            "text/plain");
+  builder.SetFileAttachment("second", "minidump.dmp", &reader2, "text/plain");
-  builder.SetFileAttachment("\"third 50% silly\"",
+
-                            "test%foo.txt",
+  FileReader reader3;
-                            ascii_http_body_path,
+  ASSERT_TRUE(reader3.Open(ascii_http_body_path));
-                            "text/plain");
+  builder.SetFileAttachment(
+      "\"third 50% silly\"", "test%foo.txt", &reader3, "text/plain");
 
   static constexpr char kFileContents[] = "This is a test.\n";
 
@@ -187,21 +188,22 @@ TEST(HTTPMultipartBuilder, OverwriteFileAttachment) {
   builder.SetFormData("a key", kValue);
   base::FilePath testdata_path =
       TestPaths::TestDataRoot().Append(FILE_PATH_LITERAL("util/net/testdata"));
-  builder.SetFileAttachment("minidump",
+
-                            "minidump.dmp",
+  FileReader reader1;
-                            testdata_path.Append(FILE_PATH_LITERAL(
+  ASSERT_TRUE(reader1.Open(
-                                "binary_http_body.dat")),
+      testdata_path.Append(FILE_PATH_LITERAL("binary_http_body.dat"))));
-                            "");
+  builder.SetFileAttachment("minidump", "minidump.dmp", &reader1, "");
-  builder.SetFileAttachment("minidump2",
+
-                            "minidump.dmp",
+  FileReader reader2;
-                            testdata_path.Append(FILE_PATH_LITERAL(
+  ASSERT_TRUE(reader2.Open(
-                                "binary_http_body.dat")),
+      testdata_path.Append(FILE_PATH_LITERAL("binary_http_body.dat"))));
-                            "");
+  builder.SetFileAttachment("minidump2", "minidump.dmp", &reader2, "");
-  builder.SetFileAttachment("minidump",
+
-                            "minidump.dmp",
+  FileReader reader3;
-                            testdata_path.Append(FILE_PATH_LITERAL(
+  ASSERT_TRUE(reader3.Open(
-                                "ascii_http_body.txt")),
+      testdata_path.Append(FILE_PATH_LITERAL("ascii_http_body.txt"))));
-                            "text/plain");
+  builder.SetFileAttachment("minidump", "minidump.dmp", &reader3, "text/plain");
+
   std::unique_ptr<HTTPBodyStream> body(builder.GetBodyStream());
   ASSERT_TRUE(body.get());
   std::string contents = ReadStreamToString(body.get());
@@ -244,10 +246,10 @@ TEST(HTTPMultipartBuilder, SharedFormDataAndAttachmentKeyNamespace) {
   builder.SetFormData("one", kValue1);
   base::FilePath ascii_http_body_path = TestPaths::TestDataRoot().Append(
       FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt"));
-  builder.SetFileAttachment("minidump",
+
-                            "minidump.dmp",
+  FileReader reader;
-                            ascii_http_body_path,
+  ASSERT_TRUE(reader.Open(ascii_http_body_path));
-                            "");
+  builder.SetFileAttachment("minidump", "minidump.dmp", &reader, "");
   static constexpr char kValue2[] = "this is not a file";
   builder.SetFormData("minidump", kValue2);
 
@@ -278,19 +280,16 @@ TEST(HTTPMultipartBuilder, SharedFormDataAndAttachmentKeyNamespace) {
 
 TEST(HTTPMultipartBuilderDeathTest, AssertUnsafeMIMEType) {
   HTTPMultipartBuilder builder;
+  FileReader reader;
   // Invalid and potentially dangerous:
-  ASSERT_DEATH_CHECK(
+  ASSERT_DEATH_CHECK(builder.SetFileAttachment("", "", &reader, "\r\n"), "");
-      builder.SetFileAttachment("", "", base::FilePath(), "\r\n"), "");
+  ASSERT_DEATH_CHECK(builder.SetFileAttachment("", "", &reader, "\""), "");
-  ASSERT_DEATH_CHECK(
+  ASSERT_DEATH_CHECK(builder.SetFileAttachment("", "", &reader, "\x12"), "");
-      builder.SetFileAttachment("", "", base::FilePath(), "\""), "");
+  ASSERT_DEATH_CHECK(builder.SetFileAttachment("", "", &reader, "<>"), "");
-  ASSERT_DEATH_CHECK(
-      builder.SetFileAttachment("", "", base::FilePath(), "\x12"), "");
-  ASSERT_DEATH_CHECK(
-      builder.SetFileAttachment("", "", base::FilePath(), "<>"), "");
   // Invalid but safe:
-  builder.SetFileAttachment("", "", base::FilePath(), "0/totally/-invalid.pdf");
+  builder.SetFileAttachment("", "", &reader, "0/totally/-invalid.pdf");
   // Valid and safe:
-  builder.SetFileAttachment("", "", base::FilePath(), "application/xml+xhtml");
+  builder.SetFileAttachment("", "", &reader, "application/xml+xhtml");
 }
 
 }  // namespace