diff --git a/client/crash_report_database.cc b/client/crash_report_database.cc index f04a526f4..9c8ba429e 100644 --- a/client/crash_report_database.cc +++ b/client/crash_report_database.cc @@ -321,6 +321,19 @@ bool CrashReportDatabase::Envelope::Initialize(const base::FilePath& path) { CrashReportDatabase::Envelope::Envelope(const UUID& uuid) : uuid_(uuid) {} +// static +bool CrashReportDatabase::Envelope::IsEvent(const base::FilePath& attachment) { + const base::FilePath::StringType basename = attachment.BaseName().value(); + return basename == FILE_PATH_LITERAL("__sentry-event"); +} + +// static +bool CrashReportDatabase::Envelope::IsBreadcrumb( + const base::FilePath& attachment) { + const base::FilePath::StringType basename = attachment.BaseName().value(); + return basename.rfind(FILE_PATH_LITERAL("__sentry-breadcrumb"), 0) == 0; +} + void CrashReportDatabase::Envelope::AddAttachments( const std::vector& attachments) { base::FilePath event; @@ -328,14 +341,9 @@ void CrashReportDatabase::Envelope::AddAttachments( std::vector others; for (const auto& attachment : attachments) { -#if BUILDFLAG(IS_WIN) - std::string basename = base::WideToUTF8(attachment.BaseName().value()); -#else - std::string basename = attachment.BaseName().value(); -#endif - if (basename == "__sentry-event") { + if (IsEvent(attachment)) { event = attachment; - } else if (basename.rfind("__sentry-breadcrumb", 0) == 0) { + } else if (IsBreadcrumb(attachment)) { breadcrumbs.push_back(attachment); } else { others.push_back(attachment); diff --git a/client/crash_report_database.h b/client/crash_report_database.h index d47853418..1e4ccccd2 100644 --- a/client/crash_report_database.h +++ b/client/crash_report_database.h @@ -218,6 +218,12 @@ class CrashReportDatabase { //! \param[in] reader File reader for the minidump data. void AddMinidump(FileReaderInterface* reader); + //! \brief Returns `true` if \a attachment is the event attachment. + static bool IsEvent(const base::FilePath& attachment); + + //! \brief Returns `true` if \a attachment is a breadcrumb attachment. + static bool IsBreadcrumb(const base::FilePath& attachment); + //! \brief Finalizes the feedback report and closes file handles. void Finish(); diff --git a/client/crashpad_client.h b/client/crashpad_client.h index 8067c4e93..a09ddf1fa 100644 --- a/client/crashpad_client.h +++ b/client/crashpad_client.h @@ -23,6 +23,7 @@ #include +#include "base/containers/span.h" #include "base/files/file_path.h" #include "build/build_config.h" #include "util/file/file_io.h" @@ -865,6 +866,14 @@ class CrashpadClient { //! \param[in] attachment The path to the file to be added. void AddAttachment(const base::FilePath& attachment); + //! \brief Writes content to an attachment file. + bool WriteAttachment( + const base::FilePath& attachment, base::span data); + + //! \brief Appends content to an attachment file. + bool AppendAttachment( + const base::FilePath& attachment, base::span data); + //! \brief Removes a file from the list of files to be attached to the crash //! report. //! diff --git a/client/crashpad_client_linux.cc b/client/crashpad_client_linux.cc index 43844df03..72440a4a1 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -38,6 +39,7 @@ #include "third_party/lss/lss.h" #include "util/file/file_io.h" #include "util/file/filesystem.h" +#include "util/file/file_writer.h" #include "util/linux/exception_handler_client.h" #include "util/linux/exception_information.h" #include "util/linux/scoped_pr_set_dumpable.h" @@ -832,6 +834,33 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { signal_handler->AddAttachment(attachment); } +bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, + base::span data) { + FileWriter writer; + if (!writer.Open(attachment, + FileWriteMode::kTruncateOrCreate, + FilePermissions::kOwnerOnly) || + !writer.Write(data.data(), data.size())) { + LOG(ERROR) << "failed to write attachment " << attachment; + return false; + } + return true; +} + +bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, + base::span data) { + FileWriter writer; + if (!writer.Open(attachment, + FileWriteMode::kReuseOrCreate, + FilePermissions::kOwnerOnly) || + writer.Seek(0, SEEK_END) < 0 || + !writer.Write(data.data(), data.size())) { + LOG(ERROR) << "failed to write attachment " << attachment; + return false; + } + return true; +} + void CrashpadClient::RemoveAttachment(const base::FilePath& attachment) { auto signal_handler = RequestCrashDumpHandler::Get(); signal_handler->RemoveAttachment(attachment); diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index faf7f282d..dd8f173d8 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -29,6 +30,7 @@ #include "base/check_op.h" #include "base/logging.h" #include "base/strings/stringprintf.h" +#include "util/file/file_writer.h" #include "util/mac/mac_util.h" #include "util/mach/bootstrap.h" #include "util/mach/child_port_handshake.h" @@ -637,6 +639,33 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { attachment.value()); } +bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, + base::span data) { + FileWriter writer; + if (!writer.Open(attachment, + FileWriteMode::kTruncateOrCreate, + FilePermissions::kOwnerOnly) || + !writer.Write(data.data(), data.size())) { + LOG(ERROR) << "failed to write attachment " << attachment; + return false; + } + return true; +} + +bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, + base::span data) { + FileWriter writer; + if (!writer.Open(attachment, + FileWriteMode::kReuseOrCreate, + FilePermissions::kOwnerOnly) || + writer.Seek(0, SEEK_END) < 0 || + !writer.Write(data.data(), data.size())) { + LOG(ERROR) << "failed to write attachment " << attachment; + return false; + } + return true; +} + void CrashpadClient::RemoveAttachment(const base::FilePath& attachment) { SendClientToServerMessage(exception_port_.get(), ClientToServerMessage::kRemoveAttachment, diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 15cbe93dd..163c338c1 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -1216,19 +1216,107 @@ void CrashpadClient::SetFirstChanceExceptionHandler( } void CrashpadClient::AddAttachment(const base::FilePath& attachment) { + const size_t path_length_bytes = + (attachment.value().length() + 1) * sizeof(wchar_t); + if (path_length_bytes > kMaxPathBytes) { + LOG(ERROR) << "Path too long: " << path_length_bytes << " bytes"; + return; + } + + ClientToServerMessage message = {}; + message.type = ClientToServerMessage::kAddAttachmentV2; + message.attachment_v2.path_length_bytes = + static_cast(path_length_bytes); + + ServerToClientMessage response = {}; + SendPayloadToCrashHandlerServer(ipc_pipe_, + message, + base::as_bytes(base::make_span( + attachment.value().c_str(), + path_length_bytes / sizeof(wchar_t))), + {}, + &response); +} + +bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, + base::span data) { + const size_t path_length_bytes = + (attachment.value().length() + 1) * sizeof(wchar_t); + if (path_length_bytes > kMaxPathBytes || + data.size() > kMaxAttachmentPayloadBytes || + data.size() > UINT32_MAX - path_length_bytes) { + LOG(ERROR) << "attachment content too large"; + return false; + } + + ClientToServerMessage message = {}; + message.type = ClientToServerMessage::kWriteAttachment; + message.attachment_write.path_length_bytes = + static_cast(path_length_bytes); + message.attachment_write.payload_length_bytes = + static_cast(data.size()); + ServerToClientMessage response = {}; - SendAttachmentToCrashHandlerServer(ipc_pipe_, - ClientToServerMessage::kAddAttachmentV2, - attachment.value(), - &response); + return SendPayloadToCrashHandlerServer(ipc_pipe_, + message, + base::as_bytes(base::make_span( + attachment.value().c_str(), + path_length_bytes + / sizeof(wchar_t))), + data, + &response); +} + +bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, + base::span data) { + const size_t path_length_bytes = + (attachment.value().length() + 1) * sizeof(wchar_t); + if (path_length_bytes > kMaxPathBytes || + data.size() > kMaxAttachmentPayloadBytes || + data.size() > UINT32_MAX - path_length_bytes) { + LOG(ERROR) << "attachment content too large"; + return false; + } + + ClientToServerMessage message = {}; + message.type = ClientToServerMessage::kAppendAttachment; + message.attachment_write.path_length_bytes = + static_cast(path_length_bytes); + message.attachment_write.payload_length_bytes = + static_cast(data.size()); + + ServerToClientMessage response = {}; + return SendPayloadToCrashHandlerServer(ipc_pipe_, + message, + base::as_bytes(base::make_span( + attachment.value().c_str(), + path_length_bytes + / sizeof(wchar_t))), + data, + &response); } void CrashpadClient::RemoveAttachment(const base::FilePath& attachment) { + const size_t path_length_bytes = + (attachment.value().length() + 1) * sizeof(wchar_t); + if (path_length_bytes > kMaxPathBytes) { + LOG(ERROR) << "Path too long: " << path_length_bytes << " bytes"; + return; + } + + ClientToServerMessage message = {}; + message.type = ClientToServerMessage::kRemoveAttachmentV2; + message.attachment_v2.path_length_bytes = + static_cast(path_length_bytes); + ServerToClientMessage response = {}; - SendAttachmentToCrashHandlerServer(ipc_pipe_, - ClientToServerMessage::kRemoveAttachmentV2, - attachment.value(), - &response); + SendPayloadToCrashHandlerServer(ipc_pipe_, + message, + base::as_bytes(base::make_span( + attachment.value().c_str(), + path_length_bytes / sizeof(wchar_t))), + {}, + &response); } void CrashpadClient::RequestRetry() { diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 8e06556b0..334e66b4a 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -18,6 +18,7 @@ #include #include "base/strings/utf_string_conversions.h" +#include "base/synchronization/lock.h" #include "client/crash_report_database.h" #include "client/settings.h" #include "handler/crash_report_upload_thread.h" @@ -48,7 +49,7 @@ CrashReportExceptionHandler::CrashReportExceptionHandler( : database_(database), upload_thread_(upload_thread), process_annotations_(process_annotations), - attachments_(*attachments), + startup_attachments_(*attachments), screenshot_(screenshot), wait_for_upload_(wait_for_upload), crash_reporter_(crash_reporter), @@ -123,24 +124,32 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( return termination_code; } - for (const auto& attachment : attachments_) { - FileReader file_reader; - if (!file_reader.Open(attachment)) { - LOG(ERROR) << "attachment " << attachment - << " couldn't be opened, skipping"; - continue; - } + { + base::AutoLock scoped_lock(attachments_lock_); + std::vector all_attachments(startup_attachments_); + all_attachments.insert(all_attachments.end(), + user_attachments_.begin(), + user_attachments_.end()); - base::FilePath filename = attachment.BaseName(); - FileWriter* file_writer = - new_report->AddAttachment(base::WideToUTF8(filename.value())); - if (file_writer == nullptr) { - LOG(ERROR) << "attachment " << filename - << " couldn't be created, skipping"; - continue; - } + for (const auto& attachment : all_attachments) { + FileReader file_reader; + if (!file_reader.Open(attachment)) { + LOG(ERROR) << "attachment " << attachment + << " couldn't be opened, skipping"; + continue; + } - CopyFileContent(&file_reader, file_writer); + base::FilePath filename = attachment.BaseName(); + FileWriter* file_writer = + new_report->AddAttachment(base::WideToUTF8(filename.value())); + if (file_writer == nullptr) { + LOG(ERROR) << "attachment " << filename + << " couldn't be created, skipping"; + continue; + } + + CopyFileContent(&file_reader, file_writer); + } } if (screenshot_ && !screenshot_->empty()) { @@ -161,12 +170,23 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( crash_envelope_ && !crash_envelope_->empty(); if (has_crash_reporter) { CrashReportDatabase::Envelope envelope(new_report->ReportID()); - if (envelope.Initialize(*crash_envelope_)) { - envelope.AddAttachments(attachments_); - if (auto reader = new_report->Reader()) { - envelope.AddMinidump(reader); + { + base::AutoLock scoped_lock(attachments_lock_); + if (envelope.Initialize(*crash_envelope_)) { + std::vector attachments(startup_attachments_); + attachments.insert(attachments.end(), + user_attachments_.begin(), + user_attachments_.end()); + envelope.AddAttachments(attachments); + if (auto reader = new_report->Reader()) { + envelope.AddMinidump(reader); + } + envelope.Finish(); + } else { + has_crash_reporter = false; } - envelope.Finish(); + } + if (has_crash_reporter) { database_->LaunchCrashReporter(*crash_reporter_, *crash_envelope_); } } @@ -199,22 +219,100 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( const base::FilePath& attachment) { - auto it = std::find(attachments_.begin(), attachments_.end(), attachment); - if (it != attachments_.end()) { + base::AutoLock scoped_lock(attachments_lock_); + if (HasStartupAttachment(attachment) || HasUserAttachment(attachment)) { LOG(WARNING) << "ignoring duplicate attachment " << attachment; return; } - attachments_.push_back(attachment); + user_attachments_.push_back(attachment); +} + +bool CrashReportExceptionHandler::HasStartupAttachment( + const base::FilePath& attachment) const { + return std::find(startup_attachments_.begin(), + startup_attachments_.end(), + attachment) != + startup_attachments_.end(); +} + +bool CrashReportExceptionHandler::HasUserAttachment( + const base::FilePath& attachment) const { + return std::find(user_attachments_.begin(), + user_attachments_.end(), + attachment) != user_attachments_.end(); +} + +// Restrict privileged handler-side attachment file writes to the external +// crash report path and startup attachments (`__sentry-xxx`). +bool CrashReportExceptionHandler::IsWritableAttachment( + const base::FilePath& attachment) const { + if (crash_envelope_ && !crash_envelope_->empty() && + attachment == *crash_envelope_) { + return true; + } + + return HasStartupAttachment(attachment) && + (CrashReportDatabase::Envelope::IsEvent(attachment) || + CrashReportDatabase::Envelope::IsBreadcrumb(attachment)); +} + +void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( + const base::FilePath& attachment, + const std::string& data) { + base::AutoLock scoped_lock(attachments_lock_); + if (!IsWritableAttachment(attachment)) { + LOG(WARNING) << "ignoring unwritable attachment " << attachment; + return; + } + + FileWriter writer; + if (!writer.Open(attachment, + FileWriteMode::kTruncateOrCreate, + FilePermissions::kOwnerOnly) || + !writer.Write(data.data(), data.size())) { + LOG(ERROR) << "failed to write attachment " << attachment; + return; + } +} + +void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( + const base::FilePath& attachment, + const std::string& data) { + base::AutoLock scoped_lock(attachments_lock_); + if (!IsWritableAttachment(attachment)) { + LOG(WARNING) << "ignoring unwritable attachment " << attachment; + return; + } + + FileWriter writer; + if (!writer.Open(attachment, + FileWriteMode::kReuseOrCreate, + FilePermissions::kOwnerOnly) || + writer.Seek(0, SEEK_END) < 0 || + !writer.Write(data.data(), data.size())) { + LOG(ERROR) << "failed to write attachment " << attachment; + return; + } } void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentRemoved( const base::FilePath& attachment) { - auto it = std::find(attachments_.begin(), attachments_.end(), attachment); - if (it == attachments_.end()) { - LOG(WARNING) << "ignoring non-existent attachment " << attachment; + base::AutoLock scoped_lock(attachments_lock_); + auto startup_it = std::find( + startup_attachments_.begin(), startup_attachments_.end(), attachment); + if (startup_it != startup_attachments_.end()) { + startup_attachments_.erase(startup_it); return; } - attachments_.erase(it); + + auto user_it = std::find( + user_attachments_.begin(), user_attachments_.end(), attachment); + if (user_it != user_attachments_.end()) { + user_attachments_.erase(user_it); + return; + } + + LOG(WARNING) << "ignoring non-existent attachment " << attachment; } void CrashReportExceptionHandler::ExceptionHandlerServerRetryRequested() { diff --git a/handler/win/crash_report_exception_handler.h b/handler/win/crash_report_exception_handler.h index 9fe36b00d..429507cb6 100644 --- a/handler/win/crash_report_exception_handler.h +++ b/handler/win/crash_report_exception_handler.h @@ -20,6 +20,7 @@ #include #include +#include "base/synchronization/lock.h" #include "handler/user_stream_data_source.h" #include "util/misc/uuid.h" #include "util/win/exception_handler_server.h" @@ -85,15 +86,25 @@ class CrashReportExceptionHandler final WinVMAddress debug_critical_section_address) override; void ExceptionHandlerServerAttachmentAdded( const base::FilePath& attachment) override; + void ExceptionHandlerServerAttachmentWritten( + const base::FilePath& attachment, const std::string& data) override; + void ExceptionHandlerServerAttachmentAppended( + const base::FilePath& attachment, const std::string& data) override; void ExceptionHandlerServerAttachmentRemoved( const base::FilePath& attachment) override; void ExceptionHandlerServerRetryRequested() override; private: + bool HasStartupAttachment(const base::FilePath& attachment) const; + bool HasUserAttachment(const base::FilePath& attachment) const; + bool IsWritableAttachment(const base::FilePath& attachment) const; + CrashReportDatabase* database_; // weak CrashReportUploadThread* upload_thread_; // weak const std::map* process_annotations_; // weak - std::vector attachments_; + base::Lock attachments_lock_; + std::vector startup_attachments_; + std::vector user_attachments_; const base::FilePath* screenshot_; // weak const bool wait_for_upload_; const base::FilePath* crash_reporter_; // weak diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index fa7b03639..24d04bbb0 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -464,12 +464,22 @@ static bool RuntimeMessageOriginIsOwner( return true; } +static void WriteResponse( + const internal::PipeServiceContext& service_context, + const ServerToClientMessage& response = {}) { + if (LoggingWriteFile(service_context.pipe(), &response, sizeof(response)) && + !FlushFileBuffers(service_context.pipe())) { + PLOG(ERROR) << "FlushFileBuffers"; + } +} + static void HandleAddAttachmentV2( const internal::PipeServiceContext& service_context, const ClientToServerMessage& message) { const uint32_t path_length_bytes = message.attachment_v2.path_length_bytes; - if (path_length_bytes == 0 || path_length_bytes > kMaxPathBytes) { + if (path_length_bytes <= sizeof(wchar_t) || + path_length_bytes > kMaxPathBytes) { LOG(ERROR) << "Invalid path length: " << path_length_bytes; return; } @@ -490,13 +500,9 @@ static void HandleAddAttachmentV2( path_buffer[path_buffer.size() - 1] = L'\0'; - ServerToClientMessage response = {}; service_context.delegate()->ExceptionHandlerServerAttachmentAdded( base::FilePath(std::wstring(path_buffer.data()))); - if (LoggingWriteFile(service_context.pipe(), &response, sizeof(response)) && - !FlushFileBuffers(service_context.pipe())) { - PLOG(ERROR) << "FlushFileBuffers"; - } + WriteResponse(service_context); } static void HandleRemoveAttachmentV2( @@ -504,7 +510,8 @@ static void HandleRemoveAttachmentV2( const ClientToServerMessage& message) { const uint32_t path_length_bytes = message.attachment_v2.path_length_bytes; - if (path_length_bytes == 0 || path_length_bytes > kMaxPathBytes) { + if (path_length_bytes <= sizeof(wchar_t) || + path_length_bytes > kMaxPathBytes) { LOG(ERROR) << "Invalid path length: " << path_length_bytes; return; } @@ -525,13 +532,92 @@ static void HandleRemoveAttachmentV2( path_buffer[path_buffer.size() - 1] = L'\0'; - ServerToClientMessage response = {}; service_context.delegate()->ExceptionHandlerServerAttachmentRemoved( base::FilePath(std::wstring(path_buffer.data()))); - if (LoggingWriteFile(service_context.pipe(), &response, sizeof(response)) && - !FlushFileBuffers(service_context.pipe())) { - PLOG(ERROR) << "FlushFileBuffers"; + WriteResponse(service_context); +} + +static bool ReadAttachment( + const internal::PipeServiceContext& service_context, + const ClientToServerMessage& message, + base::FilePath* attachment, + std::string* payload) { + const uint32_t path_length_bytes = + message.attachment_write.path_length_bytes; + const uint32_t payload_length_bytes = + message.attachment_write.payload_length_bytes; + + if (path_length_bytes <= sizeof(wchar_t) || + path_length_bytes > kMaxPathBytes || + path_length_bytes % sizeof(wchar_t) != 0) { + LOG(ERROR) << "Invalid path length: " << path_length_bytes; + return false; + } + if (payload_length_bytes > kMaxAttachmentPayloadBytes) { + LOG(ERROR) << "Invalid attachment payload length: " + << payload_length_bytes; + return false; + } + if (payload_length_bytes > UINT32_MAX - path_length_bytes) { + LOG(ERROR) << "Invalid attachment write length"; + return false; } + + std::wstring path(path_length_bytes / sizeof(wchar_t), L'\0'); + if (!LoggingReadFileExactly( + service_context.pipe(), &path[0], path_length_bytes)) { + LOG(ERROR) << "Failed to read attachment path"; + return false; + } + path.resize(path.size() - 1); + + std::string data(payload_length_bytes, '\0'); + if (payload_length_bytes > 0 && + !LoggingReadFileExactly( + service_context.pipe(), &data[0], payload_length_bytes)) { + LOG(ERROR) << "Failed to read attachment payload"; + return false; + } + + *attachment = base::FilePath(path); + *payload = std::move(data); + return true; +} + +static void HandleWriteAttachment( + const internal::PipeServiceContext& service_context, + const ClientToServerMessage& message) { + base::FilePath attachment; + std::string payload; + if (!ReadAttachment( + service_context, message, &attachment, &payload)) { + return; + } + + // Acknowledge IPC payload acceptance before disk I/O. This message exists to + // offload potentially slow disk I/O from the client; waiting for the file + // write would make the client block on the work this API is meant to avoid. + WriteResponse(service_context); + service_context.delegate()->ExceptionHandlerServerAttachmentWritten( + attachment, payload); +} + +static void HandleAppendAttachment( + const internal::PipeServiceContext& service_context, + const ClientToServerMessage& message) { + base::FilePath attachment; + std::string payload; + if (!ReadAttachment( + service_context, message, &attachment, &payload)) { + return; + } + + // Acknowledge IPC payload acceptance before disk I/O. This message exists to + // offload potentially slow disk I/O from the client; waiting for the file + // append would make the client block on the work this API is meant to avoid. + WriteResponse(service_context); + service_context.delegate()->ExceptionHandlerServerAttachmentAppended( + attachment, payload); } // This function must be called with service_context.pipe() already connected to @@ -617,6 +703,22 @@ bool ExceptionHandlerServer::ServiceClientConnection( return false; } + case ClientToServerMessage::kWriteAttachment: { + if (!RuntimeMessageOriginIsOwner(service_context)) { + return false; + } + HandleWriteAttachment(service_context, message); + return false; + } + + case ClientToServerMessage::kAppendAttachment: { + if (!RuntimeMessageOriginIsOwner(service_context)) { + return false; + } + HandleAppendAttachment(service_context, message); + return false; + } + case ClientToServerMessage::kRequestRetry: { if (!RuntimeMessageOriginIsOwner(service_context)) { return false; diff --git a/util/win/exception_handler_server.h b/util/win/exception_handler_server.h index ecea045bd..e56b2723a 100644 --- a/util/win/exception_handler_server.h +++ b/util/win/exception_handler_server.h @@ -65,6 +65,16 @@ class ExceptionHandlerServer { virtual void ExceptionHandlerServerAttachmentAdded( const base::FilePath& attachment) = 0; + //! \brief Called when the server has received a request to write an + //! attachment's contents. + virtual void ExceptionHandlerServerAttachmentWritten( + const base::FilePath& attachment, const std::string& data) = 0; + + //! \brief Called when the server has received a request to append to an + //! attachment's contents. + virtual void ExceptionHandlerServerAttachmentAppended( + const base::FilePath& attachment, const std::string& data) = 0; + //! \brief Called when the server has received a request to remove an //! attachment. //! diff --git a/util/win/registration_protocol_win.cc b/util/win/registration_protocol_win.cc index 8b0c445a4..107ec1376 100644 --- a/util/win/registration_protocol_win.cc +++ b/util/win/registration_protocol_win.cc @@ -23,6 +23,7 @@ #include "base/check.h" #include "base/logging.h" +#include "base/numerics/safe_conversions.h" #include "util/win/exception_handler_server.h" #include "util/win/loader_lock.h" #include "util/win/scoped_handle.h" @@ -141,30 +142,12 @@ bool SendToCrashHandlerServer(const std::wstring& pipe_name, } } -bool SendAttachmentToCrashHandlerServer( +bool SendPayloadToCrashHandlerServer( const std::wstring& pipe_name, - ClientToServerMessage::Type message_type, - const std::wstring& path, + const ClientToServerMessage& message, + base::span head, + base::span tail, ServerToClientMessage* response) { - if (message_type != ClientToServerMessage::kAddAttachmentV2 && - message_type != ClientToServerMessage::kRemoveAttachmentV2) { - LOG(ERROR) << "Invalid message type for attachment: " << message_type; - return false; - } - - const size_t path_length_bytes = (path.length() + 1) * sizeof(wchar_t); - - if (path_length_bytes > kMaxPathBytes) { - LOG(ERROR) << "Path too long: " << path_length_bytes << " bytes"; - return false; - } - - // Build the message header. - ClientToServerMessage message = {}; - message.type = message_type; - message.attachment_v2.path_length_bytes = - static_cast(path_length_bytes); - // Retry CreateFile() in a loop (follows the logic in // SendToCrashHandlerServer). for (;;) { @@ -201,9 +184,17 @@ bool SendAttachmentToCrashHandlerServer( return false; } - if (!WriteFile( - pipe.get(), path.c_str(), static_cast(path_length_bytes))) { - PLOG(ERROR) << "WriteFile (path)"; + if (!head.empty() && + !WriteFile( + pipe.get(), head.data(), base::checked_cast(head.size()))) { + PLOG(ERROR) << "WriteFile (payload head)"; + return false; + } + + if (!tail.empty() && + !WriteFile( + pipe.get(), tail.data(), base::checked_cast(tail.size()))) { + PLOG(ERROR) << "WriteFile (payload tail)"; return false; } diff --git a/util/win/registration_protocol_win.h b/util/win/registration_protocol_win.h index 5bf569ac0..d9ae2911a 100644 --- a/util/win/registration_protocol_win.h +++ b/util/win/registration_protocol_win.h @@ -20,6 +20,7 @@ #include +#include "base/containers/span.h" #include "util/win/address_types.h" #include "util/win/registration_protocol_win_structs.h" @@ -36,22 +37,24 @@ bool SendToCrashHandlerServer(const std::wstring& pipe_name, const ClientToServerMessage& message, ServerToClientMessage* response); -//! \brief Connect over the given \a pipe_name, passing a variable-length -//! attachment message to the server. +//! \brief Connect over the given \a pipe_name, passing a message with a +//! variable-length payload to the server. //! -//! This is used for kAddAttachmentV2 and kRemoveAttachmentV2 message types -//! which support paths longer than MAX_PATH. +//! This is used for messages whose `ClientToServerMessage` header is followed +//! by additional payload bytes. //! //! \param[in] pipe_name The name of the pipe to connect to. -//! \param[in] message_type Either kAddAttachmentV2 or kRemoveAttachmentV2. -//! \param[in] path The attachment path to send. +//! \param[in] message The message header to send. +//! \param[in] head The first payload bytes to send after \a message. +//! \param[in] tail Payload bytes to send after \a head. //! \param[out] response The server's response. //! //! \return `true` on success, `false` on failure with a message logged. -bool SendAttachmentToCrashHandlerServer( +bool SendPayloadToCrashHandlerServer( const std::wstring& pipe_name, - ClientToServerMessage::Type message_type, - const std::wstring& path, + const ClientToServerMessage& message, + base::span head, + base::span tail, ServerToClientMessage* response); //! \brief Wraps CreateNamedPipe() to create a single named pipe instance. diff --git a/util/win/registration_protocol_win_structs.h b/util/win/registration_protocol_win_structs.h index 61f95f7b4..2c7b3246c 100644 --- a/util/win/registration_protocol_win_structs.h +++ b/util/win/registration_protocol_win_structs.h @@ -132,10 +132,27 @@ struct AttachmentRequestV2 { uint32_t path_length_bytes; }; +//! \brief A variable-length attachment write request header. +//! +//! For kWriteAttachment and kAppendAttachment, the message consists of a +//! ClientToServerMessage with this header in the union, followed by +//! path_length_bytes of wchar_t data containing the null-terminated path and +//! payload_length_bytes of attachment content. +struct AttachmentWriteRequest { + //! \brief Length of the path in bytes, including null terminator. + uint32_t path_length_bytes; + + //! \brief Length of the attachment content in bytes. + uint32_t payload_length_bytes; +}; + //! follow the maximum path length documented here: //! https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation constexpr uint32_t kMaxPathBytes = 32768 * sizeof(wchar_t); +//! \brief Maximum variable-length attachment content payload, in bytes. +constexpr uint32_t kMaxAttachmentPayloadBytes = 100 * 1024 * 1024; + //! \brief The message passed from client to server by //! SendToCrashHandlerServer(). struct ClientToServerMessage { @@ -167,6 +184,12 @@ struct ClientToServerMessage { //! \brief Requests that the server retry pending report uploads. No //! additional payload. kRequestRetry, + + //! \brief For AttachmentWriteRequest. + kWriteAttachment, + + //! \brief For AttachmentWriteRequest. + kAppendAttachment, } type; union { @@ -174,6 +197,7 @@ struct ClientToServerMessage { ShutdownRequest shutdown; AttachmentRequest attachment; AttachmentRequestV2 attachment_v2; + AttachmentWriteRequest attachment_write; }; };