From 68a0e736c651283b6a431ba22c4edd4c945953f7 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 17 Oct 2017 08:50:58 -0700 Subject: [PATCH] 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 Reviewed-by: Mark Mentovai --- handler/crash_report_upload_thread.cc | 33 ++++++---- tools/crashpad_http_upload.cc | 11 +++- util/net/http_body.cc | 36 +++-------- util/net/http_body.h | 30 ++++----- util/net/http_body_test.cc | 30 ++++----- util/net/http_multipart_builder.cc | 6 +- util/net/http_multipart_builder.h | 11 ++-- util/net/http_multipart_builder_test.cc | 81 ++++++++++++------------- 8 files changed, 114 insertions(+), 124 deletions(-) diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 6842ebb2..7038108a 100644 --- a/handler/crash_report_upload_thread.cc +++ b/handler/crash_report_upload_thread.cc @@ -70,7 +70,7 @@ void InsertOrReplaceMapEntry(std::map* map, // // In the event of an error reading the minidump file, a message will be logged. std::map BreakpadHTTPFormParametersFromMinidump( - FileReader* minidump_file_reader) { + FileReaderInterface* minidump_file_reader) { ProcessSnapshotMinidump minidump_process_snapshot; if (!minidump_process_snapshot.Initialize(minidump_file_reader)) { return std::map(); @@ -344,18 +344,25 @@ CrashReportUploadThread::UploadResult CrashReportUploadThread::UploadReport( std::string* response_body) { std::map parameters; - { - FileReader minidump_file_reader; - if (!minidump_file_reader.Open(report->file_path)) { - // If the minidump file can’t be opened, all hope is lost. - return UploadResult::kPermanentFailure; - } + FileReader minidump_file_reader; + if (!minidump_file_reader.Open(report->file_path)) { + // If the minidump file can’t be opened, all hope is lost. + return UploadResult::kPermanentFailure; + } - // If the minidump file could be opened, ignore any errors that might occur - // when attempting to interpret it. 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); + FileOffset start_offset = minidump_file_reader.SeekGet(); + if (start_offset < 0) { + return UploadResult::kPermanentFailure; + } + + // 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 http_transport(HTTPTransport::Create()); diff --git a/tools/crashpad_http_upload.cc b/tools/crashpad_http_upload.cc index f8a85c43..624aaa71 100644 --- a/tools/crashpad_http_upload.cc +++ b/tools/crashpad_http_upload.cc @@ -19,9 +19,11 @@ #include #include +#include #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> 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* 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: { diff --git a/util/net/http_body.cc b/util/net/http_body.cc index 96a7cc40..7abe3a49 100644 --- a/util/net/http_body.cc +++ b/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) - : HTTPBodyStream(), path_(path), file_(), file_state_(kUnopenedFile) { +FileReaderHTTPBodyStream::FileReaderHTTPBodyStream(FileReaderInterface* reader) + : HTTPBodyStream(), reader_(reader), reached_eof_(false) { + DCHECK(reader_); } -FileHTTPBodyStream::~FileHTTPBodyStream() { -} +FileReaderHTTPBodyStream::~FileReaderHTTPBodyStream() {} -FileOperationResult FileHTTPBodyStream::GetBytesBuffer(uint8_t* buffer, - size_t max_len) { - switch (file_state_) { - case kUnopenedFile: - 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 FileReaderHTTPBodyStream::GetBytesBuffer(uint8_t* buffer, + size_t max_len) { + if (reached_eof_) { + return 0; } - FileOperationResult rv = ReadFile(file_.get(), buffer, max_len); + FileOperationResult rv = reader_->Read(buffer, max_len); if (rv == 0) { - file_.reset(); - file_state_ = kClosedAtEOF; - } else if (rv < 0) { - PLOG(ERROR) << "read"; + reached_eof_ = true; } return rv; } diff --git a/util/net/http_body.h b/util/net/http_body.h index 4fca573c..e053d9cd 100644 --- a/util/net/http_body.h +++ b/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 -//! file and provides its contents for an HTTP body. -class FileHTTPBodyStream : public HTTPBodyStream { +//! \brief An implementation of HTTPBodyStream that reads from a +//! FileReaderInterface and provides its contents for an HTTP body. +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. - explicit FileHTTPBodyStream(const base::FilePath& path); + //! \param[in] reader A FileReaderInterface from which this HTTPBodyStream + //! will read. + explicit FileReaderHTTPBodyStream(FileReaderInterface* reader); - ~FileHTTPBodyStream() override; + ~FileReaderHTTPBodyStream() override; // HTTPBodyStream: FileOperationResult GetBytesBuffer(uint8_t* buffer, size_t max_len) override; private: - enum FileState { - kUnopenedFile, - kFileOpenError, - kClosedAtEOF, - kReading, - }; + FileReaderInterface* reader_; // weak + bool reached_eof_; - base::FilePath path_; - ScopedFileHandle file_; - FileState file_state_; - - DISALLOW_COPY_AND_ASSIGN(FileHTTPBodyStream); + DISALLOW_COPY_AND_ASSIGN(FileReaderHTTPBodyStream); }; //! \brief An implementation of HTTPBodyStream that combines an array of diff --git a/util/net/http_body_test.cc b/util/net/http_body_test.cc index 93f165b1..e48cbf91 100644 --- a/util/net/http_body_test.cc +++ b/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 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); diff --git a/util/net/http_multipart_builder.cc b/util/net/http_multipart_builder.cc index cd88e430..267960b2 100644 --- a/util/net/http_multipart_builder.cc +++ b/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 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)); } diff --git a/util/net/http_multipart_builder.h b/util/net/http_multipart_builder.h index e0c98fa4..c7c59a77 100644 --- a/util/net/http_multipart_builder.h +++ b/util/net/http_multipart_builder.h @@ -19,8 +19,8 @@ #include #include -#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 diff --git a/util/net/http_multipart_builder_test.cc b/util/net/http_multipart_builder_test.cc index 71b2fa84..08806c37 100644 --- a/util/net/http_multipart_builder_test.cc +++ b/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", - ascii_http_body_path, - ""); - builder.SetFileAttachment("second", - "minidump.dmp", - ascii_http_body_path, - "text/plain"); - builder.SetFileAttachment("\"third 50% silly\"", - "test%foo.txt", - ascii_http_body_path, - "text/plain"); + + FileReader reader1; + ASSERT_TRUE(reader1.Open(ascii_http_body_path)); + builder.SetFileAttachment("first", "minidump.dmp", &reader1, ""); + + FileReader reader2; + ASSERT_TRUE(reader2.Open(ascii_http_body_path)); + builder.SetFileAttachment("second", "minidump.dmp", &reader2, "text/plain"); + + FileReader reader3; + ASSERT_TRUE(reader3.Open(ascii_http_body_path)); + 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", - testdata_path.Append(FILE_PATH_LITERAL( - "binary_http_body.dat")), - ""); - builder.SetFileAttachment("minidump2", - "minidump.dmp", - testdata_path.Append(FILE_PATH_LITERAL( - "binary_http_body.dat")), - ""); - builder.SetFileAttachment("minidump", - "minidump.dmp", - testdata_path.Append(FILE_PATH_LITERAL( - "ascii_http_body.txt")), - "text/plain"); + + FileReader reader1; + ASSERT_TRUE(reader1.Open( + testdata_path.Append(FILE_PATH_LITERAL("binary_http_body.dat")))); + builder.SetFileAttachment("minidump", "minidump.dmp", &reader1, ""); + + FileReader reader2; + ASSERT_TRUE(reader2.Open( + testdata_path.Append(FILE_PATH_LITERAL("binary_http_body.dat")))); + builder.SetFileAttachment("minidump2", "minidump.dmp", &reader2, ""); + + FileReader reader3; + ASSERT_TRUE(reader3.Open( + testdata_path.Append(FILE_PATH_LITERAL("ascii_http_body.txt")))); + builder.SetFileAttachment("minidump", "minidump.dmp", &reader3, "text/plain"); + std::unique_ptr 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", - ascii_http_body_path, - ""); + + FileReader reader; + 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( - builder.SetFileAttachment("", "", base::FilePath(), "\r\n"), ""); - ASSERT_DEATH_CHECK( - builder.SetFileAttachment("", "", base::FilePath(), "\""), ""); - ASSERT_DEATH_CHECK( - builder.SetFileAttachment("", "", base::FilePath(), "\x12"), ""); - ASSERT_DEATH_CHECK( - builder.SetFileAttachment("", "", base::FilePath(), "<>"), ""); + ASSERT_DEATH_CHECK(builder.SetFileAttachment("", "", &reader, "\r\n"), ""); + ASSERT_DEATH_CHECK(builder.SetFileAttachment("", "", &reader, "\""), ""); + ASSERT_DEATH_CHECK(builder.SetFileAttachment("", "", &reader, "\x12"), ""); + ASSERT_DEATH_CHECK(builder.SetFileAttachment("", "", &reader, "<>"), ""); // 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