From 9d13043209f07c69ee07d0635ab62e6e10c075e5 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Sun, 5 Jul 2026 17:19:56 +0200 Subject: [PATCH 01/19] feat: Add attachment update IPC Allow clients to send replacement attachment contents to the handler with variable-length IPC messages. The handler writes the updated bytes to the attachment path while keeping attachment registration unchanged. Co-Authored-By: OpenAI Codex --- client/crashpad_client.h | 8 ++ client/crashpad_client_linux.cc | 24 ++++ client/crashpad_client_mac.cc | 24 ++++ client/crashpad_client_win.cc | 106 ++++++++++++++++-- .../linux/crash_report_exception_handler.cc | 26 +++++ .../linux/crash_report_exception_handler.h | 4 + handler/linux/exception_handler_server.cc | 59 +++++++++- handler/linux/exception_handler_server.h | 9 ++ handler/mac/crash_report_exception_handler.cc | 25 +++++ handler/mac/crash_report_exception_handler.h | 4 + handler/mac/exception_handler_server.cc | 23 ++++ handler/mac/exception_handler_server.h | 12 ++ handler/win/crash_report_exception_handler.cc | 25 +++++ handler/win/crash_report_exception_handler.h | 4 + util/linux/exception_handler_client.cc | 85 ++++++++++++++ util/linux/exception_handler_client.h | 10 ++ util/linux/exception_handler_protocol.h | 19 ++++ util/linux/socket.cc | 8 +- util/linux/socket.h | 3 +- util/mach/exception_handler_protocol.h | 4 + util/win/exception_handler_server.cc | 58 ++++++++++ util/win/exception_handler_server.h | 10 ++ util/win/registration_protocol_win.cc | 32 ++---- util/win/registration_protocol_win.h | 20 ++-- util/win/registration_protocol_win_structs.h | 26 +++++ 25 files changed, 581 insertions(+), 47 deletions(-) diff --git a/client/crashpad_client.h b/client/crashpad_client.h index 8067c4e93b..55f4a89c85 100644 --- a/client/crashpad_client.h +++ b/client/crashpad_client.h @@ -865,6 +865,14 @@ class CrashpadClient { //! \param[in] attachment The path to the file to be added. void AddAttachment(const base::FilePath& attachment); + //! \brief Writes content to a handler-side attachment file. + bool WriteAttachment( + const base::FilePath& attachment, const std::string& data); + + //! \brief Appends content to a handler-side attachment file. + bool AppendAttachment( + const base::FilePath& attachment, const std::string& 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 43844df03c..7d8abbc49a 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -440,6 +440,18 @@ class RequestCrashDumpHandler : public SignalHandler { client.AddAttachment(attachment); } + bool WriteAttachment(const base::FilePath& attachment, + const std::string& data) { + ExceptionHandlerClient client(sock_to_handler_.get(), true); + return client.WriteAttachment(attachment, data); + } + + bool AppendAttachment(const base::FilePath& attachment, + const std::string& data) { + ExceptionHandlerClient client(sock_to_handler_.get(), true); + return client.AppendAttachment(attachment, data); + } + void RemoveAttachment(const base::FilePath& attachment) { ExceptionHandlerClient client(sock_to_handler_.get(), true); client.RemoveAttachment(attachment); @@ -832,6 +844,18 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { signal_handler->AddAttachment(attachment); } +bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, + const std::string& data) { + auto signal_handler = RequestCrashDumpHandler::Get(); + return signal_handler->WriteAttachment(attachment, data); +} + +bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, + const std::string& data) { + auto signal_handler = RequestCrashDumpHandler::Get(); + return signal_handler->AppendAttachment(attachment, data); +} + 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 faf7f282df..4badecbc4d 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -637,6 +637,30 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { attachment.value()); } +bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, + const std::string& data) { + std::string payload; + payload.push_back('\0'); + payload.append(attachment.value()); + payload.push_back('\0'); + payload.append(data); + return SendClientToServerMessage(exception_port_.get(), + ClientToServerMessage::kWriteAttachment, + payload); +} + +bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, + const std::string& data) { + std::string payload; + payload.push_back('\1'); + payload.append(attachment.value()); + payload.push_back('\0'); + payload.append(data); + return SendClientToServerMessage(exception_port_.get(), + ClientToServerMessage::kWriteAttachment, + payload); +} + 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 15cbe93ddc..f00af7899b 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -1216,19 +1216,109 @@ 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, + attachment.value().c_str(), + static_cast(path_length_bytes), + &response); +} + +bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, + const std::string& data) { + const size_t path_length_bytes = + (attachment.value().length() + 1) * sizeof(wchar_t); + if (path_length_bytes > kMaxPathBytes || + 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()); + message.attachment_write.operation = kAttachmentWriteReplace; + + std::string payload(path_length_bytes + data.size(), '\0'); + memcpy(&payload[0], attachment.value().c_str(), path_length_bytes); + if (!data.empty()) { + memcpy(&payload[path_length_bytes], data.data(), data.size()); + } + ServerToClientMessage response = {}; - SendAttachmentToCrashHandlerServer(ipc_pipe_, - ClientToServerMessage::kAddAttachmentV2, - attachment.value(), - &response); + return SendPayloadToCrashHandlerServer(ipc_pipe_, + message, + payload.data(), + static_cast(payload.size()), + &response); +} + +bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, + const std::string& data) { + const size_t path_length_bytes = + (attachment.value().length() + 1) * sizeof(wchar_t); + if (path_length_bytes > kMaxPathBytes || + 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()); + message.attachment_write.operation = kAttachmentWriteAppend; + + std::string payload(path_length_bytes + data.size(), '\0'); + memcpy(&payload[0], attachment.value().c_str(), path_length_bytes); + if (!data.empty()) { + memcpy(&payload[path_length_bytes], data.data(), data.size()); + } + + ServerToClientMessage response = {}; + return SendPayloadToCrashHandlerServer(ipc_pipe_, + message, + payload.data(), + static_cast(payload.size()), + &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, + attachment.value().c_str(), + static_cast(path_length_bytes), + &response); } void CrashpadClient::RequestRetry() { diff --git a/handler/linux/crash_report_exception_handler.cc b/handler/linux/crash_report_exception_handler.cc index 11f0f47176..19c3ced455 100644 --- a/handler/linux/crash_report_exception_handler.cc +++ b/handler/linux/crash_report_exception_handler.cc @@ -26,6 +26,7 @@ #include "snapshot/sanitized/process_snapshot_sanitized.h" #include "util/file/file_helper.h" #include "util/file/file_reader.h" +#include "util/file/file_writer.h" #include "util/file/output_stream_file_writer.h" #include "util/linux/direct_ptrace_connection.h" #include "util/linux/ptrace_client.h" @@ -226,6 +227,31 @@ void CrashReportExceptionHandler::AddAttachment( attachments_.push_back(attachment); } +void CrashReportExceptionHandler::WriteAttachment( + const base::FilePath& attachment, const std::string& 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; + } +} + +void CrashReportExceptionHandler::AppendAttachment( + const base::FilePath& attachment, const std::string& 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; + } +} + void CrashReportExceptionHandler::RemoveAttachment( const base::FilePath& attachment) { auto it = std::find(attachments_.begin(), attachments_.end(), attachment); diff --git a/handler/linux/crash_report_exception_handler.h b/handler/linux/crash_report_exception_handler.h index 269e00c4c7..b7034d6c8b 100644 --- a/handler/linux/crash_report_exception_handler.h +++ b/handler/linux/crash_report_exception_handler.h @@ -99,6 +99,10 @@ class CrashReportExceptionHandler : public ExceptionHandlerServer::Delegate { UUID* local_report_id = nullptr) override; void AddAttachment(const base::FilePath& attachment) override; + void WriteAttachment( + const base::FilePath& attachment, const std::string& data) override; + void AppendAttachment( + const base::FilePath& attachment, const std::string& data) override; void RemoveAttachment(const base::FilePath& attachment) override; void RequestRetry() override; diff --git a/handler/linux/exception_handler_server.cc b/handler/linux/exception_handler_server.cc index 82a185624c..6c53c256f1 100644 --- a/handler/linux/exception_handler_server.cc +++ b/handler/linux/exception_handler_server.cc @@ -24,6 +24,7 @@ #include #include +#include #include "base/check_op.h" #include "base/compiler_specific.h" @@ -452,13 +453,31 @@ bool ExceptionHandlerServer::UninstallClientSocket(Event* event) { } bool ExceptionHandlerServer::ReceiveClientMessage(Event* event) { - ExceptionHandlerProtocol::ClientToServerMessage message; + std::vector message_buffer( + sizeof(ExceptionHandlerProtocol::ClientToServerMessage) + PATH_MAX + + ExceptionHandlerProtocol::kMaxAttachmentWritePayloadSize); ucred creds; + ssize_t bytes_received = 0; if (!UnixCredentialSocket::RecvMsg( - event->fd.get(), &message, sizeof(message), &creds)) { + event->fd.get(), + message_buffer.data(), + message_buffer.size(), + &creds, + nullptr, + &bytes_received)) { + return false; + } + if (bytes_received < + static_cast( + sizeof(ExceptionHandlerProtocol::ClientToServerMessage))) { + LOG(ERROR) << "short client message"; return false; } + const auto& message = + *reinterpret_cast( + message_buffer.data()); + switch (message.type) { case ExceptionHandlerProtocol::ClientToServerMessage::kTypeCheckCredentials: return SendCredentials(event->fd.get()); @@ -491,6 +510,42 @@ bool ExceptionHandlerServer::ReceiveClientMessage(Event* event) { } delegate_->RequestRetry(); return true; + + case ExceptionHandlerProtocol::ClientToServerMessage:: + kTypeWriteAttachment: { + const uint32_t path_size = message.attachment_write_info.path_size; + const uint32_t payload_size = + message.attachment_write_info.payload_size; + const auto operation = static_cast< + ExceptionHandlerProtocol::AttachmentWriteOperation>( + message.attachment_write_info.operation); + if (path_size == 0 || path_size > PATH_MAX || + payload_size > + ExceptionHandlerProtocol::kMaxAttachmentWritePayloadSize) { + LOG(ERROR) << "invalid attachment write message"; + return true; + } + const size_t expected_size = + sizeof(ExceptionHandlerProtocol::ClientToServerMessage) + path_size + + payload_size; + if (bytes_received != static_cast(expected_size)) { + LOG(ERROR) << "truncated attachment write message"; + return true; + } + + const char* path = message_buffer.data() + + sizeof( + ExceptionHandlerProtocol::ClientToServerMessage); + const char* payload = path + path_size; + if (operation == ExceptionHandlerProtocol::kAttachmentWriteAppend) { + delegate_->AppendAttachment( + base::FilePath(path), std::string(payload, payload_size)); + } else { + delegate_->WriteAttachment( + base::FilePath(path), std::string(payload, payload_size)); + } + return true; + } } DCHECK(false); diff --git a/handler/linux/exception_handler_server.h b/handler/linux/exception_handler_server.h index 09aeab461b..c021a257a0 100644 --- a/handler/linux/exception_handler_server.h +++ b/handler/linux/exception_handler_server.h @@ -20,6 +20,7 @@ #include #include +#include #include #include "util/file/file_io.h" @@ -114,6 +115,14 @@ class ExceptionHandlerServer { //! \brief Called to add an attachment to the crash report. virtual void AddAttachment(const base::FilePath& attachment) = 0; + //! \brief Called to write an attachment's contents. + virtual void WriteAttachment( + const base::FilePath& attachment, const std::string& data) = 0; + + //! \brief Called to append to an attachment's contents. + virtual void AppendAttachment( + const base::FilePath& attachment, const std::string& data) = 0; + //! \brief Called to remove an attachment from the crash report. virtual void RemoveAttachment(const base::FilePath& attachment) = 0; diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index a6341abc8a..cdb240b445 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -336,6 +336,31 @@ void CrashReportExceptionHandler::AddAttachment( attachments_.push_back(attachment); } +void CrashReportExceptionHandler::WriteAttachment( + const base::FilePath& attachment, const std::string& 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; + } +} + +void CrashReportExceptionHandler::AppendAttachment( + const base::FilePath& attachment, const std::string& 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; + } +} + void CrashReportExceptionHandler::RemoveAttachment( const base::FilePath& attachment) { auto it = std::find(attachments_.begin(), attachments_.end(), attachment); diff --git a/handler/mac/crash_report_exception_handler.h b/handler/mac/crash_report_exception_handler.h index 2fe4ba0393..5af5ccefbd 100644 --- a/handler/mac/crash_report_exception_handler.h +++ b/handler/mac/crash_report_exception_handler.h @@ -99,6 +99,10 @@ class CrashReportExceptionHandler final // ExceptionHandlerServer::Delegate: void RequestRetry() override; void AddAttachment(const base::FilePath& attachment) override; + void WriteAttachment( + const base::FilePath& attachment, const std::string& data) override; + void AppendAttachment( + const base::FilePath& attachment, const std::string& data) override; void RemoveAttachment(const base::FilePath& attachment) override; private: diff --git a/handler/mac/exception_handler_server.cc b/handler/mac/exception_handler_server.cc index bdcae577ed..3ab47bf142 100644 --- a/handler/mac/exception_handler_server.cc +++ b/handler/mac/exception_handler_server.cc @@ -65,6 +65,29 @@ class ClientToServerMessageServer : public MachMessageServer::Interface { case ClientToServerMessage::kAddAttachment: delegate_->AddAttachment(base::FilePath(message->Payload())); break; + case ClientToServerMessage::kWriteAttachment: { + std::string payload = message->Payload(); + if (payload.empty()) { + LOG(ERROR) << "malformed attachment write message"; + break; + } + bool should_append = payload[0] != '\0'; + size_t path_end = payload.find('\0', 1); + if (path_end == std::string::npos) { + LOG(ERROR) << "malformed attachment write message"; + break; + } + if (should_append) { + delegate_->AppendAttachment( + base::FilePath(payload.substr(1, path_end - 1)), + payload.substr(path_end + 1)); + } else { + delegate_->WriteAttachment( + base::FilePath(payload.substr(1, path_end - 1)), + payload.substr(path_end + 1)); + } + break; + } case ClientToServerMessage::kRemoveAttachment: delegate_->RemoveAttachment(base::FilePath(message->Payload())); break; diff --git a/handler/mac/exception_handler_server.h b/handler/mac/exception_handler_server.h index 5f7bd8337b..472e81bf96 100644 --- a/handler/mac/exception_handler_server.h +++ b/handler/mac/exception_handler_server.h @@ -18,6 +18,8 @@ #include #include +#include + #include "base/apple/scoped_mach_port.h" #include "base/files/file_path.h" #include "util/mach/exc_server_variants.h" @@ -42,6 +44,16 @@ class ExceptionHandlerServer { //! the list of files attached to crash reports. virtual void AddAttachment(const base::FilePath& attachment) = 0; + //! \brief Called when the server has received a request to write an + //! attachment's contents. + virtual void WriteAttachment( + 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 AppendAttachment( + const base::FilePath& attachment, const std::string& data) = 0; + //! \brief Called when the server has received a request to remove a file //! from the list of files attached to crash reports. virtual void RemoveAttachment(const base::FilePath& attachment) = 0; diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 8e06556b05..b562d02f66 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -207,6 +207,31 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( attachments_.push_back(attachment); } +void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( + const base::FilePath& attachment, const std::string& 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; + } +} + +void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( + const base::FilePath& attachment, const std::string& 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; + } +} + void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentRemoved( const base::FilePath& attachment) { auto it = std::find(attachments_.begin(), attachments_.end(), attachment); diff --git a/handler/win/crash_report_exception_handler.h b/handler/win/crash_report_exception_handler.h index 9fe36b00dd..a0443481c9 100644 --- a/handler/win/crash_report_exception_handler.h +++ b/handler/win/crash_report_exception_handler.h @@ -85,6 +85,10 @@ 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; diff --git a/util/linux/exception_handler_client.cc b/util/linux/exception_handler_client.cc index 306a932e38..3398fb3eee 100644 --- a/util/linux/exception_handler_client.cc +++ b/util/linux/exception_handler_client.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -26,6 +27,7 @@ #include "build/build_config.h" #include "third_party/lss/lss.h" #include "util/file/file_io.h" +#include "util/file/file_writer.h" #include "util/linux/ptrace_broker.h" #include "util/linux/socket.h" #include "util/misc/from_pointer_cast.h" @@ -62,6 +64,71 @@ class ScopedSigprocmaskRestore { bool mask_is_set_; }; +bool WriteAttachmentFile( + const base::FilePath& attachment, + const std::string& data, + ExceptionHandlerProtocol::AttachmentWriteOperation operation) { + FileWriter writer; + if (!writer.Open(attachment, + operation == ExceptionHandlerProtocol::kAttachmentWriteAppend + ? FileWriteMode::kReuseOrCreate + : FileWriteMode::kTruncateOrCreate, + FilePermissions::kOwnerOnly) || + (operation == ExceptionHandlerProtocol::kAttachmentWriteAppend && + writer.Seek(0, SEEK_END) < 0) || + !writer.Write(data.data(), data.size())) { + LOG(ERROR) << "failed to write attachment " << attachment; + return false; + } + return true; +} + +bool SendAttachmentWrite( + int server_sock, + const base::FilePath& attachment, + const std::string& data, + ExceptionHandlerProtocol::AttachmentWriteOperation operation) { + if (attachment.value().size() + 1 > PATH_MAX) { + LOG(ERROR) << "attachment path too long: " << attachment.value().size() + << " bytes"; + return false; + } + if (data.size() > + ExceptionHandlerProtocol::kMaxAttachmentWritePayloadSize) { + return WriteAttachmentFile(attachment, data, operation); + } + + ExceptionHandlerProtocol::ClientToServerMessage message; + message.type = + ExceptionHandlerProtocol::ClientToServerMessage::kTypeWriteAttachment; + message.attachment_write_info.path_size = + static_cast(attachment.value().size() + 1); + message.attachment_write_info.payload_size = + static_cast(data.size()); + message.attachment_write_info.operation = operation; + + std::string payload(sizeof(message) + attachment.value().size() + 1 + + data.size(), + '\0'); + memcpy(&payload[0], &message, sizeof(message)); + memcpy(&payload[sizeof(message)], attachment.value().c_str(), + attachment.value().size() + 1); + if (!data.empty()) { + memcpy(&payload[sizeof(message) + attachment.value().size() + 1], + data.data(), + data.size()); + } + int result = + UnixCredentialSocket::SendMsg(server_sock, payload.data(), payload.size()); + if (result == 0) { + return true; + } + if (result == EMSGSIZE || result == ENOBUFS) { + return WriteAttachmentFile(attachment, data, operation); + } + return false; +} + } // namespace ExceptionHandlerClient::ExceptionHandlerClient(int sock, bool multiple_clients) @@ -234,6 +301,24 @@ void ExceptionHandlerClient::AddAttachment(const base::FilePath& attachment) { UnixCredentialSocket::SendMsg(server_sock_, &message, sizeof(message)); } +bool ExceptionHandlerClient::WriteAttachment(const base::FilePath& attachment, + const std::string& data) { + return SendAttachmentWrite( + server_sock_, + attachment, + data, + ExceptionHandlerProtocol::kAttachmentWriteReplace); +} + +bool ExceptionHandlerClient::AppendAttachment(const base::FilePath& attachment, + const std::string& data) { + return SendAttachmentWrite( + server_sock_, + attachment, + data, + ExceptionHandlerProtocol::kAttachmentWriteAppend); +} + void ExceptionHandlerClient::RemoveAttachment( const base::FilePath& attachment) { ExceptionHandlerProtocol::ClientToServerMessage message; diff --git a/util/linux/exception_handler_client.h b/util/linux/exception_handler_client.h index a7e4c66dfb..6b74893a4f 100644 --- a/util/linux/exception_handler_client.h +++ b/util/linux/exception_handler_client.h @@ -18,6 +18,8 @@ #include #include +#include + #include "util/linux/exception_handler_protocol.h" namespace crashpad { @@ -71,6 +73,14 @@ class ExceptionHandlerClient { //! \brief Adds an attachment to the crash report. void AddAttachment(const base::FilePath& attachment); + //! \brief Requests that the handler write an attachment's contents. + bool WriteAttachment( + const base::FilePath& attachment, const std::string& data); + + //! \brief Requests that the handler append to an attachment's contents. + bool AppendAttachment( + const base::FilePath& attachment, const std::string& data); + //! \brief Removes an attachment from the crash report. void RemoveAttachment(const base::FilePath& attachment); diff --git a/util/linux/exception_handler_protocol.h b/util/linux/exception_handler_protocol.h index 0b6a642f00..afbc1f8178 100644 --- a/util/linux/exception_handler_protocol.h +++ b/util/linux/exception_handler_protocol.h @@ -64,6 +64,19 @@ class ExceptionHandlerProtocol { char path[PATH_MAX]; }; + static constexpr uint32_t kMaxAttachmentWritePayloadSize = 192 * 1024; + + enum AttachmentWriteOperation : uint32_t { + kAttachmentWriteReplace, + kAttachmentWriteAppend, + }; + + struct AttachmentWriteInformation { + uint32_t path_size; + uint32_t payload_size; + uint32_t operation; + }; + //! \brief The signal used to indicate a crash dump is complete. //! //! When multiple clients share a single socket connection with the handler, @@ -96,6 +109,9 @@ class ExceptionHandlerProtocol { //! \brief Request that the server retry pending report uploads. kTypeRequestRetry, + + //! \brief Request that the server write an attachment's contents. + kTypeWriteAttachment, }; Type type; @@ -109,6 +125,9 @@ class ExceptionHandlerProtocol { //! \brief Valid for type == kAddAttachment || type == kRemoveAttachment AttachmentInformation attachment_info; + + //! \brief Valid for type == kTypeWriteAttachment. + AttachmentWriteInformation attachment_write_info; }; }; diff --git a/util/linux/socket.cc b/util/linux/socket.cc index bad5056f9f..6641bb5205 100644 --- a/util/linux/socket.cc +++ b/util/linux/socket.cc @@ -101,7 +101,8 @@ bool UnixCredentialSocket::RecvMsg(int fd, void* buf, size_t buf_size, ucred* creds, - std::vector* fds) { + std::vector* fds, + ssize_t* bytes_received) { iovec iov; iov.iov_base = buf; iov.iov_len = buf_size; @@ -120,6 +121,9 @@ bool UnixCredentialSocket::RecvMsg(int fd, PLOG(ERROR) << "recvmsg"; return false; } + if (bytes_received) { + *bytes_received = res; + } ucred* local_creds = nullptr; std::vector local_fds; @@ -178,7 +182,7 @@ bool UnixCredentialSocket::RecvMsg(int fd, return false; } - if (static_cast(res) != buf_size) { + if (!bytes_received && static_cast(res) != buf_size) { LOG(ERROR) << "incorrect payload size " << res; return false; } diff --git a/util/linux/socket.h b/util/linux/socket.h index 4bde25c814..3545c0e99a 100644 --- a/util/linux/socket.h +++ b/util/linux/socket.h @@ -88,7 +88,8 @@ class UnixCredentialSocket { void* buf, size_t buf_size, ucred* creds, - std::vector* fds = nullptr); + std::vector* fds = nullptr, + ssize_t* bytes_received = nullptr); }; } // namespace crashpad diff --git a/util/mach/exception_handler_protocol.h b/util/mach/exception_handler_protocol.h index cdd3af1d1d..7fe9d2df85 100644 --- a/util/mach/exception_handler_protocol.h +++ b/util/mach/exception_handler_protocol.h @@ -49,6 +49,10 @@ struct ClientToServerMessage { //! \brief Remove a file from the list of files attached to crash reports. //! The payload contains the attachment path. kRemoveAttachment = 3, + + //! \brief Write an attachment's contents. The payload contains a + //! null-terminated attachment path followed by the attachment content. + kWriteAttachment = 4, }; mach_msg_header_t header; diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index 2a19023c9d..07b55660bf 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -528,6 +528,59 @@ static void HandleRemoveAttachmentV2( LoggingWriteFile(service_context.pipe(), &response, sizeof(response)); } +static void HandleWriteAttachment( + const internal::PipeServiceContext& service_context, + const ClientToServerMessage& message) { + const uint32_t path_length_bytes = + message.attachment_write.path_length_bytes; + const uint32_t payload_length_bytes = + message.attachment_write.payload_length_bytes; + const auto operation = + static_cast(message.attachment_write.operation); + + if (path_length_bytes == 0 || path_length_bytes > kMaxPathBytes || + path_length_bytes % sizeof(wchar_t) != 0) { + LOG(ERROR) << "Invalid path length: " << path_length_bytes; + return; + } + if (payload_length_bytes > UINT32_MAX - path_length_bytes) { + LOG(ERROR) << "Invalid attachment write length"; + return; + } + + const uint32_t request_payload_length_bytes = + path_length_bytes + payload_length_bytes; + std::string request_payload(request_payload_length_bytes, '\0'); + if (!LoggingReadFileExactly( + service_context.pipe(), + &request_payload[0], + request_payload_length_bytes)) { + LOG(ERROR) << "Failed to read attachment write"; + return; + } + + const size_t path_length = path_length_bytes / sizeof(wchar_t) - 1; + std::wstring path(path_length, L'\0'); + if (path_length > 0) { + memcpy(&path[0], + request_payload.data(), + path_length_bytes - sizeof(wchar_t)); + } + + std::string payload( + request_payload.data() + path_length_bytes, payload_length_bytes); + + ServerToClientMessage response = {}; + if (operation == kAttachmentWriteAppend) { + service_context.delegate()->ExceptionHandlerServerAttachmentAppended( + base::FilePath(path), payload); + } else { + service_context.delegate()->ExceptionHandlerServerAttachmentWritten( + base::FilePath(path), payload); + } + LoggingWriteFile(service_context.pipe(), &response, sizeof(response)); +} + // This function must be called with service_context.pipe() already connected to // a client pipe. It exchanges data with the client and adds a ClientData record // to service_context->clients(). @@ -611,6 +664,11 @@ bool ExceptionHandlerServer::ServiceClientConnection( return false; } + case ClientToServerMessage::kWriteAttachment: { + HandleWriteAttachment(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 ecea045bd9..e56b2723aa 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 8b0c445a41..503c00fef3 100644 --- a/util/win/registration_protocol_win.cc +++ b/util/win/registration_protocol_win.cc @@ -141,30 +141,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, + const void* payload, + uint32_t payload_size, 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 +183,9 @@ bool SendAttachmentToCrashHandlerServer( return false; } - if (!WriteFile( - pipe.get(), path.c_str(), static_cast(path_length_bytes))) { - PLOG(ERROR) << "WriteFile (path)"; + if (payload_size > 0 && + !WriteFile(pipe.get(), payload, static_cast(payload_size))) { + PLOG(ERROR) << "WriteFile (payload)"; return false; } diff --git a/util/win/registration_protocol_win.h b/util/win/registration_protocol_win.h index 5bf569ac02..90de5ea2bf 100644 --- a/util/win/registration_protocol_win.h +++ b/util/win/registration_protocol_win.h @@ -36,22 +36,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] payload The payload bytes to send after \a message. +//! \param[in] payload_size The size of \a payload. //! \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, + const void* payload, + uint32_t payload_size, 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 61f95f7b42..e565bbdea5 100644 --- a/util/win/registration_protocol_win_structs.h +++ b/util/win/registration_protocol_win_structs.h @@ -132,6 +132,28 @@ struct AttachmentRequestV2 { uint32_t path_length_bytes; }; +enum AttachmentWriteOperation : uint32_t { + kAttachmentWriteReplace, + kAttachmentWriteAppend, +}; + +//! \brief A variable-length attachment write request header. +//! +//! For kWriteAttachment, 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; + + //! \brief The write operation to apply to the attachment content. + uint32_t operation; +}; + //! 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); @@ -167,6 +189,9 @@ struct ClientToServerMessage { //! \brief Requests that the server retry pending report uploads. No //! additional payload. kRequestRetry, + + //! \brief For AttachmentWriteRequest. + kWriteAttachment, } type; union { @@ -174,6 +199,7 @@ struct ClientToServerMessage { ShutdownRequest shutdown; AttachmentRequest attachment; AttachmentRequestV2 attachment_v2; + AttachmentWriteRequest attachment_write; }; }; From c3c57547125d5a72a5b82c58f020e7e8716c12bc Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Mon, 6 Jul 2026 14:08:08 +0200 Subject: [PATCH 02/19] Drop IPC on Unix - simple file operations are fast enough --- client/crashpad_client.h | 4 +- client/crashpad_client_linux.cc | 37 ++++---- client/crashpad_client_mac.cc | 39 +++++---- .../linux/crash_report_exception_handler.cc | 26 ------ .../linux/crash_report_exception_handler.h | 4 - handler/linux/exception_handler_server.cc | 59 +------------ handler/linux/exception_handler_server.h | 9 -- handler/mac/crash_report_exception_handler.cc | 25 ------ handler/mac/crash_report_exception_handler.h | 4 - handler/mac/exception_handler_server.cc | 23 ----- handler/mac/exception_handler_server.h | 12 --- util/linux/exception_handler_client.cc | 85 ------------------- util/linux/exception_handler_client.h | 10 --- util/linux/exception_handler_protocol.h | 19 ----- util/linux/socket.cc | 8 +- util/linux/socket.h | 3 +- util/mach/exception_handler_protocol.h | 4 - 17 files changed, 50 insertions(+), 321 deletions(-) diff --git a/client/crashpad_client.h b/client/crashpad_client.h index 55f4a89c85..1bd8bd66de 100644 --- a/client/crashpad_client.h +++ b/client/crashpad_client.h @@ -865,11 +865,11 @@ class CrashpadClient { //! \param[in] attachment The path to the file to be added. void AddAttachment(const base::FilePath& attachment); - //! \brief Writes content to a handler-side attachment file. + //! \brief Writes content to an attachment file. bool WriteAttachment( const base::FilePath& attachment, const std::string& data); - //! \brief Appends content to a handler-side attachment file. + //! \brief Appends content to an attachment file. bool AppendAttachment( const base::FilePath& attachment, const std::string& data); diff --git a/client/crashpad_client_linux.cc b/client/crashpad_client_linux.cc index 7d8abbc49a..ff0d67fb8b 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" @@ -440,18 +442,6 @@ class RequestCrashDumpHandler : public SignalHandler { client.AddAttachment(attachment); } - bool WriteAttachment(const base::FilePath& attachment, - const std::string& data) { - ExceptionHandlerClient client(sock_to_handler_.get(), true); - return client.WriteAttachment(attachment, data); - } - - bool AppendAttachment(const base::FilePath& attachment, - const std::string& data) { - ExceptionHandlerClient client(sock_to_handler_.get(), true); - return client.AppendAttachment(attachment, data); - } - void RemoveAttachment(const base::FilePath& attachment) { ExceptionHandlerClient client(sock_to_handler_.get(), true); client.RemoveAttachment(attachment); @@ -846,14 +836,29 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, const std::string& data) { - auto signal_handler = RequestCrashDumpHandler::Get(); - return signal_handler->WriteAttachment(attachment, 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, const std::string& data) { - auto signal_handler = RequestCrashDumpHandler::Get(); - return signal_handler->AppendAttachment(attachment, 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) { diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index 4badecbc4d..e1e7469cc2 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -19,7 +19,8 @@ #include #include #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" @@ -639,26 +641,29 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, const std::string& data) { - std::string payload; - payload.push_back('\0'); - payload.append(attachment.value()); - payload.push_back('\0'); - payload.append(data); - return SendClientToServerMessage(exception_port_.get(), - ClientToServerMessage::kWriteAttachment, - payload); + 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, const std::string& data) { - std::string payload; - payload.push_back('\1'); - payload.append(attachment.value()); - payload.push_back('\0'); - payload.append(data); - return SendClientToServerMessage(exception_port_.get(), - ClientToServerMessage::kWriteAttachment, - payload); + 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) { diff --git a/handler/linux/crash_report_exception_handler.cc b/handler/linux/crash_report_exception_handler.cc index 19c3ced455..11f0f47176 100644 --- a/handler/linux/crash_report_exception_handler.cc +++ b/handler/linux/crash_report_exception_handler.cc @@ -26,7 +26,6 @@ #include "snapshot/sanitized/process_snapshot_sanitized.h" #include "util/file/file_helper.h" #include "util/file/file_reader.h" -#include "util/file/file_writer.h" #include "util/file/output_stream_file_writer.h" #include "util/linux/direct_ptrace_connection.h" #include "util/linux/ptrace_client.h" @@ -227,31 +226,6 @@ void CrashReportExceptionHandler::AddAttachment( attachments_.push_back(attachment); } -void CrashReportExceptionHandler::WriteAttachment( - const base::FilePath& attachment, const std::string& 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; - } -} - -void CrashReportExceptionHandler::AppendAttachment( - const base::FilePath& attachment, const std::string& 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; - } -} - void CrashReportExceptionHandler::RemoveAttachment( const base::FilePath& attachment) { auto it = std::find(attachments_.begin(), attachments_.end(), attachment); diff --git a/handler/linux/crash_report_exception_handler.h b/handler/linux/crash_report_exception_handler.h index b7034d6c8b..269e00c4c7 100644 --- a/handler/linux/crash_report_exception_handler.h +++ b/handler/linux/crash_report_exception_handler.h @@ -99,10 +99,6 @@ class CrashReportExceptionHandler : public ExceptionHandlerServer::Delegate { UUID* local_report_id = nullptr) override; void AddAttachment(const base::FilePath& attachment) override; - void WriteAttachment( - const base::FilePath& attachment, const std::string& data) override; - void AppendAttachment( - const base::FilePath& attachment, const std::string& data) override; void RemoveAttachment(const base::FilePath& attachment) override; void RequestRetry() override; diff --git a/handler/linux/exception_handler_server.cc b/handler/linux/exception_handler_server.cc index 6c53c256f1..82a185624c 100644 --- a/handler/linux/exception_handler_server.cc +++ b/handler/linux/exception_handler_server.cc @@ -24,7 +24,6 @@ #include #include -#include #include "base/check_op.h" #include "base/compiler_specific.h" @@ -453,31 +452,13 @@ bool ExceptionHandlerServer::UninstallClientSocket(Event* event) { } bool ExceptionHandlerServer::ReceiveClientMessage(Event* event) { - std::vector message_buffer( - sizeof(ExceptionHandlerProtocol::ClientToServerMessage) + PATH_MAX + - ExceptionHandlerProtocol::kMaxAttachmentWritePayloadSize); + ExceptionHandlerProtocol::ClientToServerMessage message; ucred creds; - ssize_t bytes_received = 0; if (!UnixCredentialSocket::RecvMsg( - event->fd.get(), - message_buffer.data(), - message_buffer.size(), - &creds, - nullptr, - &bytes_received)) { - return false; - } - if (bytes_received < - static_cast( - sizeof(ExceptionHandlerProtocol::ClientToServerMessage))) { - LOG(ERROR) << "short client message"; + event->fd.get(), &message, sizeof(message), &creds)) { return false; } - const auto& message = - *reinterpret_cast( - message_buffer.data()); - switch (message.type) { case ExceptionHandlerProtocol::ClientToServerMessage::kTypeCheckCredentials: return SendCredentials(event->fd.get()); @@ -510,42 +491,6 @@ bool ExceptionHandlerServer::ReceiveClientMessage(Event* event) { } delegate_->RequestRetry(); return true; - - case ExceptionHandlerProtocol::ClientToServerMessage:: - kTypeWriteAttachment: { - const uint32_t path_size = message.attachment_write_info.path_size; - const uint32_t payload_size = - message.attachment_write_info.payload_size; - const auto operation = static_cast< - ExceptionHandlerProtocol::AttachmentWriteOperation>( - message.attachment_write_info.operation); - if (path_size == 0 || path_size > PATH_MAX || - payload_size > - ExceptionHandlerProtocol::kMaxAttachmentWritePayloadSize) { - LOG(ERROR) << "invalid attachment write message"; - return true; - } - const size_t expected_size = - sizeof(ExceptionHandlerProtocol::ClientToServerMessage) + path_size + - payload_size; - if (bytes_received != static_cast(expected_size)) { - LOG(ERROR) << "truncated attachment write message"; - return true; - } - - const char* path = message_buffer.data() + - sizeof( - ExceptionHandlerProtocol::ClientToServerMessage); - const char* payload = path + path_size; - if (operation == ExceptionHandlerProtocol::kAttachmentWriteAppend) { - delegate_->AppendAttachment( - base::FilePath(path), std::string(payload, payload_size)); - } else { - delegate_->WriteAttachment( - base::FilePath(path), std::string(payload, payload_size)); - } - return true; - } } DCHECK(false); diff --git a/handler/linux/exception_handler_server.h b/handler/linux/exception_handler_server.h index c021a257a0..09aeab461b 100644 --- a/handler/linux/exception_handler_server.h +++ b/handler/linux/exception_handler_server.h @@ -20,7 +20,6 @@ #include #include -#include #include #include "util/file/file_io.h" @@ -115,14 +114,6 @@ class ExceptionHandlerServer { //! \brief Called to add an attachment to the crash report. virtual void AddAttachment(const base::FilePath& attachment) = 0; - //! \brief Called to write an attachment's contents. - virtual void WriteAttachment( - const base::FilePath& attachment, const std::string& data) = 0; - - //! \brief Called to append to an attachment's contents. - virtual void AppendAttachment( - const base::FilePath& attachment, const std::string& data) = 0; - //! \brief Called to remove an attachment from the crash report. virtual void RemoveAttachment(const base::FilePath& attachment) = 0; diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index cdb240b445..a6341abc8a 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -336,31 +336,6 @@ void CrashReportExceptionHandler::AddAttachment( attachments_.push_back(attachment); } -void CrashReportExceptionHandler::WriteAttachment( - const base::FilePath& attachment, const std::string& 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; - } -} - -void CrashReportExceptionHandler::AppendAttachment( - const base::FilePath& attachment, const std::string& 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; - } -} - void CrashReportExceptionHandler::RemoveAttachment( const base::FilePath& attachment) { auto it = std::find(attachments_.begin(), attachments_.end(), attachment); diff --git a/handler/mac/crash_report_exception_handler.h b/handler/mac/crash_report_exception_handler.h index 5af5ccefbd..2fe4ba0393 100644 --- a/handler/mac/crash_report_exception_handler.h +++ b/handler/mac/crash_report_exception_handler.h @@ -99,10 +99,6 @@ class CrashReportExceptionHandler final // ExceptionHandlerServer::Delegate: void RequestRetry() override; void AddAttachment(const base::FilePath& attachment) override; - void WriteAttachment( - const base::FilePath& attachment, const std::string& data) override; - void AppendAttachment( - const base::FilePath& attachment, const std::string& data) override; void RemoveAttachment(const base::FilePath& attachment) override; private: diff --git a/handler/mac/exception_handler_server.cc b/handler/mac/exception_handler_server.cc index 3ab47bf142..bdcae577ed 100644 --- a/handler/mac/exception_handler_server.cc +++ b/handler/mac/exception_handler_server.cc @@ -65,29 +65,6 @@ class ClientToServerMessageServer : public MachMessageServer::Interface { case ClientToServerMessage::kAddAttachment: delegate_->AddAttachment(base::FilePath(message->Payload())); break; - case ClientToServerMessage::kWriteAttachment: { - std::string payload = message->Payload(); - if (payload.empty()) { - LOG(ERROR) << "malformed attachment write message"; - break; - } - bool should_append = payload[0] != '\0'; - size_t path_end = payload.find('\0', 1); - if (path_end == std::string::npos) { - LOG(ERROR) << "malformed attachment write message"; - break; - } - if (should_append) { - delegate_->AppendAttachment( - base::FilePath(payload.substr(1, path_end - 1)), - payload.substr(path_end + 1)); - } else { - delegate_->WriteAttachment( - base::FilePath(payload.substr(1, path_end - 1)), - payload.substr(path_end + 1)); - } - break; - } case ClientToServerMessage::kRemoveAttachment: delegate_->RemoveAttachment(base::FilePath(message->Payload())); break; diff --git a/handler/mac/exception_handler_server.h b/handler/mac/exception_handler_server.h index 472e81bf96..5f7bd8337b 100644 --- a/handler/mac/exception_handler_server.h +++ b/handler/mac/exception_handler_server.h @@ -18,8 +18,6 @@ #include #include -#include - #include "base/apple/scoped_mach_port.h" #include "base/files/file_path.h" #include "util/mach/exc_server_variants.h" @@ -44,16 +42,6 @@ class ExceptionHandlerServer { //! the list of files attached to crash reports. virtual void AddAttachment(const base::FilePath& attachment) = 0; - //! \brief Called when the server has received a request to write an - //! attachment's contents. - virtual void WriteAttachment( - 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 AppendAttachment( - const base::FilePath& attachment, const std::string& data) = 0; - //! \brief Called when the server has received a request to remove a file //! from the list of files attached to crash reports. virtual void RemoveAttachment(const base::FilePath& attachment) = 0; diff --git a/util/linux/exception_handler_client.cc b/util/linux/exception_handler_client.cc index 3398fb3eee..306a932e38 100644 --- a/util/linux/exception_handler_client.cc +++ b/util/linux/exception_handler_client.cc @@ -16,7 +16,6 @@ #include #include -#include #include #include #include @@ -27,7 +26,6 @@ #include "build/build_config.h" #include "third_party/lss/lss.h" #include "util/file/file_io.h" -#include "util/file/file_writer.h" #include "util/linux/ptrace_broker.h" #include "util/linux/socket.h" #include "util/misc/from_pointer_cast.h" @@ -64,71 +62,6 @@ class ScopedSigprocmaskRestore { bool mask_is_set_; }; -bool WriteAttachmentFile( - const base::FilePath& attachment, - const std::string& data, - ExceptionHandlerProtocol::AttachmentWriteOperation operation) { - FileWriter writer; - if (!writer.Open(attachment, - operation == ExceptionHandlerProtocol::kAttachmentWriteAppend - ? FileWriteMode::kReuseOrCreate - : FileWriteMode::kTruncateOrCreate, - FilePermissions::kOwnerOnly) || - (operation == ExceptionHandlerProtocol::kAttachmentWriteAppend && - writer.Seek(0, SEEK_END) < 0) || - !writer.Write(data.data(), data.size())) { - LOG(ERROR) << "failed to write attachment " << attachment; - return false; - } - return true; -} - -bool SendAttachmentWrite( - int server_sock, - const base::FilePath& attachment, - const std::string& data, - ExceptionHandlerProtocol::AttachmentWriteOperation operation) { - if (attachment.value().size() + 1 > PATH_MAX) { - LOG(ERROR) << "attachment path too long: " << attachment.value().size() - << " bytes"; - return false; - } - if (data.size() > - ExceptionHandlerProtocol::kMaxAttachmentWritePayloadSize) { - return WriteAttachmentFile(attachment, data, operation); - } - - ExceptionHandlerProtocol::ClientToServerMessage message; - message.type = - ExceptionHandlerProtocol::ClientToServerMessage::kTypeWriteAttachment; - message.attachment_write_info.path_size = - static_cast(attachment.value().size() + 1); - message.attachment_write_info.payload_size = - static_cast(data.size()); - message.attachment_write_info.operation = operation; - - std::string payload(sizeof(message) + attachment.value().size() + 1 + - data.size(), - '\0'); - memcpy(&payload[0], &message, sizeof(message)); - memcpy(&payload[sizeof(message)], attachment.value().c_str(), - attachment.value().size() + 1); - if (!data.empty()) { - memcpy(&payload[sizeof(message) + attachment.value().size() + 1], - data.data(), - data.size()); - } - int result = - UnixCredentialSocket::SendMsg(server_sock, payload.data(), payload.size()); - if (result == 0) { - return true; - } - if (result == EMSGSIZE || result == ENOBUFS) { - return WriteAttachmentFile(attachment, data, operation); - } - return false; -} - } // namespace ExceptionHandlerClient::ExceptionHandlerClient(int sock, bool multiple_clients) @@ -301,24 +234,6 @@ void ExceptionHandlerClient::AddAttachment(const base::FilePath& attachment) { UnixCredentialSocket::SendMsg(server_sock_, &message, sizeof(message)); } -bool ExceptionHandlerClient::WriteAttachment(const base::FilePath& attachment, - const std::string& data) { - return SendAttachmentWrite( - server_sock_, - attachment, - data, - ExceptionHandlerProtocol::kAttachmentWriteReplace); -} - -bool ExceptionHandlerClient::AppendAttachment(const base::FilePath& attachment, - const std::string& data) { - return SendAttachmentWrite( - server_sock_, - attachment, - data, - ExceptionHandlerProtocol::kAttachmentWriteAppend); -} - void ExceptionHandlerClient::RemoveAttachment( const base::FilePath& attachment) { ExceptionHandlerProtocol::ClientToServerMessage message; diff --git a/util/linux/exception_handler_client.h b/util/linux/exception_handler_client.h index 6b74893a4f..a7e4c66dfb 100644 --- a/util/linux/exception_handler_client.h +++ b/util/linux/exception_handler_client.h @@ -18,8 +18,6 @@ #include #include -#include - #include "util/linux/exception_handler_protocol.h" namespace crashpad { @@ -73,14 +71,6 @@ class ExceptionHandlerClient { //! \brief Adds an attachment to the crash report. void AddAttachment(const base::FilePath& attachment); - //! \brief Requests that the handler write an attachment's contents. - bool WriteAttachment( - const base::FilePath& attachment, const std::string& data); - - //! \brief Requests that the handler append to an attachment's contents. - bool AppendAttachment( - const base::FilePath& attachment, const std::string& data); - //! \brief Removes an attachment from the crash report. void RemoveAttachment(const base::FilePath& attachment); diff --git a/util/linux/exception_handler_protocol.h b/util/linux/exception_handler_protocol.h index afbc1f8178..0b6a642f00 100644 --- a/util/linux/exception_handler_protocol.h +++ b/util/linux/exception_handler_protocol.h @@ -64,19 +64,6 @@ class ExceptionHandlerProtocol { char path[PATH_MAX]; }; - static constexpr uint32_t kMaxAttachmentWritePayloadSize = 192 * 1024; - - enum AttachmentWriteOperation : uint32_t { - kAttachmentWriteReplace, - kAttachmentWriteAppend, - }; - - struct AttachmentWriteInformation { - uint32_t path_size; - uint32_t payload_size; - uint32_t operation; - }; - //! \brief The signal used to indicate a crash dump is complete. //! //! When multiple clients share a single socket connection with the handler, @@ -109,9 +96,6 @@ class ExceptionHandlerProtocol { //! \brief Request that the server retry pending report uploads. kTypeRequestRetry, - - //! \brief Request that the server write an attachment's contents. - kTypeWriteAttachment, }; Type type; @@ -125,9 +109,6 @@ class ExceptionHandlerProtocol { //! \brief Valid for type == kAddAttachment || type == kRemoveAttachment AttachmentInformation attachment_info; - - //! \brief Valid for type == kTypeWriteAttachment. - AttachmentWriteInformation attachment_write_info; }; }; diff --git a/util/linux/socket.cc b/util/linux/socket.cc index 6641bb5205..bad5056f9f 100644 --- a/util/linux/socket.cc +++ b/util/linux/socket.cc @@ -101,8 +101,7 @@ bool UnixCredentialSocket::RecvMsg(int fd, void* buf, size_t buf_size, ucred* creds, - std::vector* fds, - ssize_t* bytes_received) { + std::vector* fds) { iovec iov; iov.iov_base = buf; iov.iov_len = buf_size; @@ -121,9 +120,6 @@ bool UnixCredentialSocket::RecvMsg(int fd, PLOG(ERROR) << "recvmsg"; return false; } - if (bytes_received) { - *bytes_received = res; - } ucred* local_creds = nullptr; std::vector local_fds; @@ -182,7 +178,7 @@ bool UnixCredentialSocket::RecvMsg(int fd, return false; } - if (!bytes_received && static_cast(res) != buf_size) { + if (static_cast(res) != buf_size) { LOG(ERROR) << "incorrect payload size " << res; return false; } diff --git a/util/linux/socket.h b/util/linux/socket.h index 3545c0e99a..4bde25c814 100644 --- a/util/linux/socket.h +++ b/util/linux/socket.h @@ -88,8 +88,7 @@ class UnixCredentialSocket { void* buf, size_t buf_size, ucred* creds, - std::vector* fds = nullptr, - ssize_t* bytes_received = nullptr); + std::vector* fds = nullptr); }; } // namespace crashpad diff --git a/util/mach/exception_handler_protocol.h b/util/mach/exception_handler_protocol.h index 7fe9d2df85..cdd3af1d1d 100644 --- a/util/mach/exception_handler_protocol.h +++ b/util/mach/exception_handler_protocol.h @@ -49,10 +49,6 @@ struct ClientToServerMessage { //! \brief Remove a file from the list of files attached to crash reports. //! The payload contains the attachment path. kRemoveAttachment = 3, - - //! \brief Write an attachment's contents. The payload contains a - //! null-terminated attachment path followed by the attachment content. - kWriteAttachment = 4, }; mach_msg_header_t header; From a2950822f0704c44357b9e610cd9b0d10082a905 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Mon, 6 Jul 2026 15:11:38 +0200 Subject: [PATCH 03/19] Clean up Windows --- client/crashpad_client_win.cc | 4 +- util/win/exception_handler_server.cc | 60 +++++++++++++++----- util/win/registration_protocol_win_structs.h | 19 +++---- 3 files changed, 53 insertions(+), 30 deletions(-) diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index f00af7899b..402bd673cb 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -1252,7 +1252,6 @@ bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, static_cast(path_length_bytes); message.attachment_write.payload_length_bytes = static_cast(data.size()); - message.attachment_write.operation = kAttachmentWriteReplace; std::string payload(path_length_bytes + data.size(), '\0'); memcpy(&payload[0], attachment.value().c_str(), path_length_bytes); @@ -1279,12 +1278,11 @@ bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, } ClientToServerMessage message = {}; - message.type = ClientToServerMessage::kWriteAttachment; + 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()); - message.attachment_write.operation = kAttachmentWriteAppend; std::string payload(path_length_bytes + data.size(), '\0'); memcpy(&payload[0], attachment.value().c_str(), path_length_bytes); diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index 07b55660bf..a89e9bcb0c 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -528,24 +528,24 @@ static void HandleRemoveAttachmentV2( LoggingWriteFile(service_context.pipe(), &response, sizeof(response)); } -static void HandleWriteAttachment( +static bool ReadAttachment( const internal::PipeServiceContext& service_context, - const ClientToServerMessage& message) { + 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; - const auto operation = - static_cast(message.attachment_write.operation); if (path_length_bytes == 0 || path_length_bytes > kMaxPathBytes || path_length_bytes % sizeof(wchar_t) != 0) { LOG(ERROR) << "Invalid path length: " << path_length_bytes; - return; + return false; } if (payload_length_bytes > UINT32_MAX - path_length_bytes) { LOG(ERROR) << "Invalid attachment write length"; - return; + return false; } const uint32_t request_payload_length_bytes = @@ -556,7 +556,7 @@ static void HandleWriteAttachment( &request_payload[0], request_payload_length_bytes)) { LOG(ERROR) << "Failed to read attachment write"; - return; + return false; } const size_t path_length = path_length_bytes / sizeof(wchar_t) - 1; @@ -567,17 +567,42 @@ static void HandleWriteAttachment( path_length_bytes - sizeof(wchar_t)); } - std::string payload( - request_payload.data() + path_length_bytes, payload_length_bytes); + *attachment = base::FilePath(path); + *payload = + std::string(request_payload.data() + path_length_bytes, + payload_length_bytes); + 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; + } ServerToClientMessage response = {}; - if (operation == kAttachmentWriteAppend) { - service_context.delegate()->ExceptionHandlerServerAttachmentAppended( - base::FilePath(path), payload); - } else { - service_context.delegate()->ExceptionHandlerServerAttachmentWritten( - base::FilePath(path), payload); + service_context.delegate()->ExceptionHandlerServerAttachmentWritten( + attachment, payload); + LoggingWriteFile(service_context.pipe(), &response, sizeof(response)); +} + +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; } + + ServerToClientMessage response = {}; + service_context.delegate()->ExceptionHandlerServerAttachmentAppended( + attachment, payload); LoggingWriteFile(service_context.pipe(), &response, sizeof(response)); } @@ -669,6 +694,11 @@ bool ExceptionHandlerServer::ServiceClientConnection( return false; } + case ClientToServerMessage::kAppendAttachment: { + HandleAppendAttachment(service_context, message); + return false; + } + case ClientToServerMessage::kRequestRetry: { if (!RuntimeMessageOriginIsOwner(service_context)) { return false; diff --git a/util/win/registration_protocol_win_structs.h b/util/win/registration_protocol_win_structs.h index e565bbdea5..96a378de9b 100644 --- a/util/win/registration_protocol_win_structs.h +++ b/util/win/registration_protocol_win_structs.h @@ -132,26 +132,18 @@ struct AttachmentRequestV2 { uint32_t path_length_bytes; }; -enum AttachmentWriteOperation : uint32_t { - kAttachmentWriteReplace, - kAttachmentWriteAppend, -}; - //! \brief A variable-length attachment write request header. //! -//! For kWriteAttachment, 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. +//! 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; - - //! \brief The write operation to apply to the attachment content. - uint32_t operation; }; //! follow the maximum path length documented here: @@ -192,6 +184,9 @@ struct ClientToServerMessage { //! \brief For AttachmentWriteRequest. kWriteAttachment, + + //! \brief For AttachmentWriteRequest. + kAppendAttachment, } type; union { From 63c674c5de04a7e3ad9465e28b6572e7270b2e4c Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Wed, 8 Jul 2026 10:03:35 +0200 Subject: [PATCH 04/19] don't wait for write - defeats the purpose --- util/win/exception_handler_server.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index a89e9bcb0c..47712dbb75 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -585,9 +585,11 @@ static void HandleWriteAttachment( } ServerToClientMessage response = {}; + if (!LoggingWriteFile(service_context.pipe(), &response, sizeof(response))) { + return; + } service_context.delegate()->ExceptionHandlerServerAttachmentWritten( attachment, payload); - LoggingWriteFile(service_context.pipe(), &response, sizeof(response)); } static void HandleAppendAttachment( @@ -601,9 +603,11 @@ static void HandleAppendAttachment( } ServerToClientMessage response = {}; + if (!LoggingWriteFile(service_context.pipe(), &response, sizeof(response))) { + return; + } service_context.delegate()->ExceptionHandlerServerAttachmentAppended( attachment, payload); - LoggingWriteFile(service_context.pipe(), &response, sizeof(response)); } // This function must be called with service_context.pipe() already connected to From 2185824c0e0222d43e03eae64d29a86ee2ffc809 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Wed, 8 Jul 2026 10:18:32 +0200 Subject: [PATCH 05/19] cap payload size --- client/crashpad_client_win.cc | 2 ++ util/win/exception_handler_server.cc | 5 +++++ util/win/registration_protocol_win_structs.h | 3 +++ 3 files changed, 10 insertions(+) diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 402bd673cb..00e8697144 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -1241,6 +1241,7 @@ bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, 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; @@ -1272,6 +1273,7 @@ bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, 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; diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index 47712dbb75..99df188fae 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -543,6 +543,11 @@ static bool ReadAttachment( 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; diff --git a/util/win/registration_protocol_win_structs.h b/util/win/registration_protocol_win_structs.h index 96a378de9b..2c7b3246c4 100644 --- a/util/win/registration_protocol_win_structs.h +++ b/util/win/registration_protocol_win_structs.h @@ -150,6 +150,9 @@ struct AttachmentWriteRequest { //! 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 { From 1cb278906dca8870ab817c48479715854ecdb9a4 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Fri, 10 Jul 2026 08:46:52 +0200 Subject: [PATCH 06/19] add owner checks to the new IPC methods --- util/win/exception_handler_server.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index 99df188fae..dd2d9a2ec9 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -699,11 +699,17 @@ bool ExceptionHandlerServer::ServiceClientConnection( } 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; } From 8c3f5bc729ebf4cc1b97752847ee41a7827b997e Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Fri, 10 Jul 2026 09:01:36 +0200 Subject: [PATCH 07/19] fix typo --- client/crashpad_client_mac.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index e1e7469cc2..25d5b04db4 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -20,7 +20,7 @@ #include #include #include -#include +#include #include #include From b39aaef429680dbd827d070958e1968973683e0f Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Fri, 10 Jul 2026 13:27:33 +0200 Subject: [PATCH 08/19] add clarifying comments --- util/win/exception_handler_server.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index dd2d9a2ec9..a185063ca4 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -589,6 +589,9 @@ static void HandleWriteAttachment( 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. ServerToClientMessage response = {}; if (!LoggingWriteFile(service_context.pipe(), &response, sizeof(response))) { return; @@ -607,6 +610,9 @@ static void HandleAppendAttachment( 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. ServerToClientMessage response = {}; if (!LoggingWriteFile(service_context.pipe(), &response, sizeof(response))) { return; From 09ac5c8e34ed5e4167b0be45ab088734cf051a26 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Thu, 23 Jul 2026 10:58:39 +0200 Subject: [PATCH 09/19] serialize --- handler/win/crash_report_exception_handler.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index b562d02f66..91d21efe48 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -17,6 +17,7 @@ #include #include +#include "base/synchronization/lock.h" #include "base/strings/utf_string_conversions.h" #include "client/crash_report_database.h" #include "client/settings.h" @@ -34,6 +35,15 @@ namespace crashpad { +namespace { + +base::Lock& AttachmentWriteLock() { + static base::Lock lock; + return lock; +} + +} // namespace + CrashReportExceptionHandler::CrashReportExceptionHandler( CrashReportDatabase* database, CrashReportUploadThread* upload_thread, @@ -209,6 +219,7 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( const base::FilePath& attachment, const std::string& data) { + base::AutoLock scoped_lock(AttachmentWriteLock()); FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kTruncateOrCreate, @@ -221,6 +232,7 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( const base::FilePath& attachment, const std::string& data) { + base::AutoLock scoped_lock(AttachmentWriteLock()); FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kReuseOrCreate, From d1afd53b8c3f774a92c121a0749673a7e5e1dffb Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Thu, 23 Jul 2026 11:10:39 +0200 Subject: [PATCH 10/19] file lock --- handler/win/crash_report_exception_handler.cc | 46 +++++++++++-------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 91d21efe48..d69e85887c 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -37,7 +37,7 @@ namespace crashpad { namespace { -base::Lock& AttachmentWriteLock() { +base::Lock& AttachmentFileLock() { static base::Lock lock; return lock; } @@ -133,24 +133,27 @@ 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(AttachmentFileLock()); + for (const auto& attachment : attachments_) { + FileReader file_reader; + if (!file_reader.Open(attachment)) { + LOG(ERROR) << "attachment " << attachment + << " couldn't be opened, skipping"; + continue; + } - 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; - } + 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); + CopyFileContent(&file_reader, file_writer); + } } if (screenshot_ && !screenshot_->empty()) { @@ -172,7 +175,10 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( if (has_crash_reporter) { CrashReportDatabase::Envelope envelope(new_report->ReportID()); if (envelope.Initialize(*crash_envelope_)) { - envelope.AddAttachments(attachments_); + { + base::AutoLock scoped_lock(AttachmentFileLock()); + envelope.AddAttachments(attachments_); + } if (auto reader = new_report->Reader()) { envelope.AddMinidump(reader); } @@ -219,7 +225,7 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( const base::FilePath& attachment, const std::string& data) { - base::AutoLock scoped_lock(AttachmentWriteLock()); + base::AutoLock scoped_lock(AttachmentFileLock()); FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kTruncateOrCreate, @@ -232,7 +238,7 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( const base::FilePath& attachment, const std::string& data) { - base::AutoLock scoped_lock(AttachmentWriteLock()); + base::AutoLock scoped_lock(AttachmentFileLock()); FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kReuseOrCreate, From 23c9d948af5ae47bdc6fbc4d4a6656b748d3f1bf Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Thu, 23 Jul 2026 11:28:15 +0200 Subject: [PATCH 11/19] flush response --- util/win/exception_handler_server.cc | 31 ++++++++++++---------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index f18d116071..d6495dcb62 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -464,6 +464,15 @@ 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) { @@ -490,13 +499,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( @@ -525,13 +530,9 @@ 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( @@ -598,10 +599,7 @@ static void HandleWriteAttachment( // 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. - ServerToClientMessage response = {}; - if (!LoggingWriteFile(service_context.pipe(), &response, sizeof(response))) { - return; - } + WriteResponse(service_context); service_context.delegate()->ExceptionHandlerServerAttachmentWritten( attachment, payload); } @@ -619,10 +617,7 @@ static void HandleAppendAttachment( // 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. - ServerToClientMessage response = {}; - if (!LoggingWriteFile(service_context.pipe(), &response, sizeof(response))) { - return; - } + WriteResponse(service_context); service_context.delegate()->ExceptionHandlerServerAttachmentAppended( attachment, payload); } From 6728ce72f2207d99cd867455f4eb310f3483305f Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Thu, 23 Jul 2026 11:40:39 +0200 Subject: [PATCH 12/19] attachments_lock_ --- handler/win/crash_report_exception_handler.cc | 19 ++++++------------- handler/win/crash_report_exception_handler.h | 2 ++ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index d69e85887c..f380502709 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -35,15 +35,6 @@ namespace crashpad { -namespace { - -base::Lock& AttachmentFileLock() { - static base::Lock lock; - return lock; -} - -} // namespace - CrashReportExceptionHandler::CrashReportExceptionHandler( CrashReportDatabase* database, CrashReportUploadThread* upload_thread, @@ -134,7 +125,7 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( } { - base::AutoLock scoped_lock(AttachmentFileLock()); + base::AutoLock scoped_lock(attachments_lock_); for (const auto& attachment : attachments_) { FileReader file_reader; if (!file_reader.Open(attachment)) { @@ -176,7 +167,7 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( CrashReportDatabase::Envelope envelope(new_report->ReportID()); if (envelope.Initialize(*crash_envelope_)) { { - base::AutoLock scoped_lock(AttachmentFileLock()); + base::AutoLock scoped_lock(attachments_lock_); envelope.AddAttachments(attachments_); } if (auto reader = new_report->Reader()) { @@ -215,6 +206,7 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( const base::FilePath& attachment) { + base::AutoLock scoped_lock(attachments_lock_); auto it = std::find(attachments_.begin(), attachments_.end(), attachment); if (it != attachments_.end()) { LOG(WARNING) << "ignoring duplicate attachment " << attachment; @@ -225,7 +217,7 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( const base::FilePath& attachment, const std::string& data) { - base::AutoLock scoped_lock(AttachmentFileLock()); + base::AutoLock scoped_lock(attachments_lock_); FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kTruncateOrCreate, @@ -238,7 +230,7 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( const base::FilePath& attachment, const std::string& data) { - base::AutoLock scoped_lock(AttachmentFileLock()); + base::AutoLock scoped_lock(attachments_lock_); FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kReuseOrCreate, @@ -252,6 +244,7 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentRemoved( const base::FilePath& attachment) { + base::AutoLock scoped_lock(attachments_lock_); auto it = std::find(attachments_.begin(), attachments_.end(), attachment); if (it == attachments_.end()) { LOG(WARNING) << "ignoring non-existent attachment " << attachment; diff --git a/handler/win/crash_report_exception_handler.h b/handler/win/crash_report_exception_handler.h index a0443481c9..890fd430fe 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" @@ -97,6 +98,7 @@ class CrashReportExceptionHandler final CrashReportDatabase* database_; // weak CrashReportUploadThread* upload_thread_; // weak const std::map* process_annotations_; // weak + base::Lock attachments_lock_; std::vector attachments_; const base::FilePath* screenshot_; // weak const bool wait_for_upload_; From b3023164ec901b065643e3f27efe600bbd11d708 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Thu, 23 Jul 2026 12:20:43 +0200 Subject: [PATCH 13/19] restrict --- handler/win/crash_report_exception_handler.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index f380502709..e8af7d5179 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -218,6 +218,12 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( const base::FilePath& attachment, const std::string& data) { base::AutoLock scoped_lock(attachments_lock_); + auto it = std::find(attachments_.begin(), attachments_.end(), attachment); + if (it == attachments_.end()) { + LOG(WARNING) << "ignoring unregistered attachment " << attachment; + return; + } + FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kTruncateOrCreate, @@ -231,6 +237,12 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( const base::FilePath& attachment, const std::string& data) { base::AutoLock scoped_lock(attachments_lock_); + auto it = std::find(attachments_.begin(), attachments_.end(), attachment); + if (it == attachments_.end()) { + LOG(WARNING) << "ignoring unregistered attachment " << attachment; + return; + } + FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kReuseOrCreate, From fca5966e2067b315638de08655d134b6ece7d031 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Thu, 23 Jul 2026 13:47:32 +0200 Subject: [PATCH 14/19] restrict more --- client/crash_report_database.cc | 22 ++++++---- client/crash_report_database.h | 6 +++ handler/win/crash_report_exception_handler.cc | 41 +++++++++++++++---- handler/win/crash_report_exception_handler.h | 2 + 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/client/crash_report_database.cc b/client/crash_report_database.cc index f04a526f43..9c8ba429ef 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 d47853418d..1e4ccccd25 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/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index e8af7d5179..1e4513f70a 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -17,8 +17,8 @@ #include #include -#include "base/synchronization/lock.h" #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" @@ -215,12 +215,35 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( attachments_.push_back(attachment); } +// Restrict privileged handler-side attachment file writes to the special +// `__sentry-xxx` and external crash report attachments that are generated +// by the backend. +bool CrashReportExceptionHandler::IsWritableAttachment( + const base::FilePath& attachment) { + if (crash_envelope_ && !crash_envelope_->empty() && + attachment == *crash_envelope_) { + return true; + } + + if (std::find(attachments_.begin(), attachments_.end(), attachment) == + attachments_.end()) { + return false; + } + + if (!CrashReportDatabase::Envelope::IsEvent(attachment) && + !CrashReportDatabase::Envelope::IsBreadcrumb(attachment)) { + return false; + } + + return true; +} + void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( - const base::FilePath& attachment, const std::string& data) { + const base::FilePath& attachment, + const std::string& data) { base::AutoLock scoped_lock(attachments_lock_); - auto it = std::find(attachments_.begin(), attachments_.end(), attachment); - if (it == attachments_.end()) { - LOG(WARNING) << "ignoring unregistered attachment " << attachment; + if (!IsWritableAttachment(attachment)) { + LOG(WARNING) << "ignoring unwritable attachment " << attachment; return; } @@ -235,11 +258,11 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( } void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( - const base::FilePath& attachment, const std::string& data) { + const base::FilePath& attachment, + const std::string& data) { base::AutoLock scoped_lock(attachments_lock_); - auto it = std::find(attachments_.begin(), attachments_.end(), attachment); - if (it == attachments_.end()) { - LOG(WARNING) << "ignoring unregistered attachment " << attachment; + if (!IsWritableAttachment(attachment)) { + LOG(WARNING) << "ignoring unwritable attachment " << attachment; return; } diff --git a/handler/win/crash_report_exception_handler.h b/handler/win/crash_report_exception_handler.h index 890fd430fe..723da5202b 100644 --- a/handler/win/crash_report_exception_handler.h +++ b/handler/win/crash_report_exception_handler.h @@ -95,6 +95,8 @@ class CrashReportExceptionHandler final void ExceptionHandlerServerRetryRequested() override; private: + bool IsWritableAttachment(const base::FilePath& attachment); + CrashReportDatabase* database_; // weak CrashReportUploadThread* upload_thread_; // weak const std::map* process_annotations_; // weak From 014b5dfa350986d8c9eb7d2965f84f50033beaad Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Thu, 23 Jul 2026 14:56:38 +0200 Subject: [PATCH 15/19] lock --- handler/win/crash_report_exception_handler.cc | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 1e4513f70a..22f6dd8cac 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -165,15 +165,19 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( crash_envelope_ && !crash_envelope_->empty(); if (has_crash_reporter) { CrashReportDatabase::Envelope envelope(new_report->ReportID()); - if (envelope.Initialize(*crash_envelope_)) { - { - base::AutoLock scoped_lock(attachments_lock_); + { + base::AutoLock scoped_lock(attachments_lock_); + if (envelope.Initialize(*crash_envelope_)) { envelope.AddAttachments(attachments_); + if (auto reader = new_report->Reader()) { + envelope.AddMinidump(reader); + } + envelope.Finish(); + } else { + has_crash_reporter = false; } - if (auto reader = new_report->Reader()) { - envelope.AddMinidump(reader); - } - envelope.Finish(); + } + if (has_crash_reporter) { database_->LaunchCrashReporter(*crash_reporter_, *crash_envelope_); } } From fde1cf56b8f44ff81494e975c61ddfb090eba03c Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Thu, 23 Jul 2026 16:35:37 +0200 Subject: [PATCH 16/19] startup vs. user attachments --- handler/win/crash_report_exception_handler.cc | 63 ++++++++++++------- handler/win/crash_report_exception_handler.h | 7 ++- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 22f6dd8cac..01608f00bd 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -49,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), @@ -126,7 +126,12 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( { base::AutoLock scoped_lock(attachments_lock_); - for (const auto& attachment : attachments_) { + std::vector all_attachments(*startup_attachments_); + all_attachments.insert(all_attachments.end(), + user_attachments_.begin(), + user_attachments_.end()); + + for (const auto& attachment : all_attachments) { FileReader file_reader; if (!file_reader.Open(attachment)) { LOG(ERROR) << "attachment " << attachment @@ -168,7 +173,11 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( { base::AutoLock scoped_lock(attachments_lock_); if (envelope.Initialize(*crash_envelope_)) { - envelope.AddAttachments(attachments_); + 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); } @@ -211,35 +220,40 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( const base::FilePath& attachment) { base::AutoLock scoped_lock(attachments_lock_); - auto it = std::find(attachments_.begin(), attachments_.end(), attachment); - if (it != attachments_.end()) { + 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 special -// `__sentry-xxx` and external crash report attachments that are generated -// by the backend. +// 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 base::FilePath& attachment) const { if (crash_envelope_ && !crash_envelope_->empty() && attachment == *crash_envelope_) { return true; } - if (std::find(attachments_.begin(), attachments_.end(), attachment) == - attachments_.end()) { - return false; - } - - if (!CrashReportDatabase::Envelope::IsEvent(attachment) && - !CrashReportDatabase::Envelope::IsBreadcrumb(attachment)) { - return false; - } - - return true; + return HasStartupAttachment(attachment) && + (CrashReportDatabase::Envelope::IsEvent(attachment) || + CrashReportDatabase::Envelope::IsBreadcrumb(attachment)); } void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentWritten( @@ -284,12 +298,13 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentRemoved( const base::FilePath& attachment) { base::AutoLock scoped_lock(attachments_lock_); - auto it = std::find(attachments_.begin(), attachments_.end(), attachment); - if (it == attachments_.end()) { + auto it = std::find( + user_attachments_.begin(), user_attachments_.end(), attachment); + if (it == user_attachments_.end()) { LOG(WARNING) << "ignoring non-existent attachment " << attachment; return; } - attachments_.erase(it); + user_attachments_.erase(it); } void CrashReportExceptionHandler::ExceptionHandlerServerRetryRequested() { diff --git a/handler/win/crash_report_exception_handler.h b/handler/win/crash_report_exception_handler.h index 723da5202b..d6f3a4989e 100644 --- a/handler/win/crash_report_exception_handler.h +++ b/handler/win/crash_report_exception_handler.h @@ -95,13 +95,16 @@ class CrashReportExceptionHandler final void ExceptionHandlerServerRetryRequested() override; private: - bool IsWritableAttachment(const base::FilePath& attachment); + 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 base::Lock attachments_lock_; - std::vector attachments_; + const std::vector* startup_attachments_; // weak + std::vector user_attachments_; const base::FilePath* screenshot_; // weak const bool wait_for_upload_; const base::FilePath* crash_reporter_; // weak From bbd95ecd7720f2995e8c12ff4bc925792fe8b862 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Fri, 24 Jul 2026 13:44:26 +0200 Subject: [PATCH 17/19] use base::span to prevent copies --- client/crashpad_client.h | 5 ++-- client/crashpad_client_linux.cc | 4 +-- client/crashpad_client_mac.cc | 4 +-- client/crashpad_client_win.cc | 42 +++++++++++++-------------- util/win/exception_handler_server.cc | 27 +++++++---------- util/win/registration_protocol_win.cc | 19 ++++++++---- util/win/registration_protocol_win.h | 9 +++--- 7 files changed, 57 insertions(+), 53 deletions(-) diff --git a/client/crashpad_client.h b/client/crashpad_client.h index 1bd8bd66de..a09ddf1faa 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" @@ -867,11 +868,11 @@ class CrashpadClient { //! \brief Writes content to an attachment file. bool WriteAttachment( - const base::FilePath& attachment, const std::string& data); + const base::FilePath& attachment, base::span data); //! \brief Appends content to an attachment file. bool AppendAttachment( - const base::FilePath& attachment, const std::string& data); + 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 ff0d67fb8b..72440a4a1f 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -835,7 +835,7 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { } bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, - const std::string& data) { + base::span data) { FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kTruncateOrCreate, @@ -848,7 +848,7 @@ bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, } bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, - const std::string& data) { + base::span data) { FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kReuseOrCreate, diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index 25d5b04db4..dd8f173d82 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -640,7 +640,7 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { } bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, - const std::string& data) { + base::span data) { FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kTruncateOrCreate, @@ -653,7 +653,7 @@ bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, } bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, - const std::string& data) { + base::span data) { FileWriter writer; if (!writer.Open(attachment, FileWriteMode::kReuseOrCreate, diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 00e8697144..163c338c15 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -1231,13 +1231,15 @@ void CrashpadClient::AddAttachment(const base::FilePath& attachment) { ServerToClientMessage response = {}; SendPayloadToCrashHandlerServer(ipc_pipe_, message, - attachment.value().c_str(), - static_cast(path_length_bytes), + base::as_bytes(base::make_span( + attachment.value().c_str(), + path_length_bytes / sizeof(wchar_t))), + {}, &response); } bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, - const std::string& data) { + base::span data) { const size_t path_length_bytes = (attachment.value().length() + 1) * sizeof(wchar_t); if (path_length_bytes > kMaxPathBytes || @@ -1254,22 +1256,19 @@ bool CrashpadClient::WriteAttachment(const base::FilePath& attachment, message.attachment_write.payload_length_bytes = static_cast(data.size()); - std::string payload(path_length_bytes + data.size(), '\0'); - memcpy(&payload[0], attachment.value().c_str(), path_length_bytes); - if (!data.empty()) { - memcpy(&payload[path_length_bytes], data.data(), data.size()); - } - ServerToClientMessage response = {}; return SendPayloadToCrashHandlerServer(ipc_pipe_, message, - payload.data(), - static_cast(payload.size()), + 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, - const std::string& data) { + base::span data) { const size_t path_length_bytes = (attachment.value().length() + 1) * sizeof(wchar_t); if (path_length_bytes > kMaxPathBytes || @@ -1286,17 +1285,14 @@ bool CrashpadClient::AppendAttachment(const base::FilePath& attachment, message.attachment_write.payload_length_bytes = static_cast(data.size()); - std::string payload(path_length_bytes + data.size(), '\0'); - memcpy(&payload[0], attachment.value().c_str(), path_length_bytes); - if (!data.empty()) { - memcpy(&payload[path_length_bytes], data.data(), data.size()); - } - ServerToClientMessage response = {}; return SendPayloadToCrashHandlerServer(ipc_pipe_, message, - payload.data(), - static_cast(payload.size()), + base::as_bytes(base::make_span( + attachment.value().c_str(), + path_length_bytes + / sizeof(wchar_t))), + data, &response); } @@ -1316,8 +1312,10 @@ void CrashpadClient::RemoveAttachment(const base::FilePath& attachment) { ServerToClientMessage response = {}; SendPayloadToCrashHandlerServer(ipc_pipe_, message, - attachment.value().c_str(), - static_cast(path_length_bytes), + base::as_bytes(base::make_span( + attachment.value().c_str(), + path_length_bytes / sizeof(wchar_t))), + {}, &response); } diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index d6495dcb62..ff1e117f6c 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -560,29 +560,24 @@ static bool ReadAttachment( return false; } - const uint32_t request_payload_length_bytes = - path_length_bytes + payload_length_bytes; - std::string request_payload(request_payload_length_bytes, '\0'); + std::wstring path(path_length_bytes / sizeof(wchar_t), L'\0'); if (!LoggingReadFileExactly( - service_context.pipe(), - &request_payload[0], - request_payload_length_bytes)) { - LOG(ERROR) << "Failed to read attachment write"; + service_context.pipe(), &path[0], path_length_bytes)) { + LOG(ERROR) << "Failed to read attachment path"; return false; } + path.resize(path.size() - 1); - const size_t path_length = path_length_bytes / sizeof(wchar_t) - 1; - std::wstring path(path_length, L'\0'); - if (path_length > 0) { - memcpy(&path[0], - request_payload.data(), - path_length_bytes - sizeof(wchar_t)); + 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::string(request_payload.data() + path_length_bytes, - payload_length_bytes); + *payload = std::move(data); return true; } diff --git a/util/win/registration_protocol_win.cc b/util/win/registration_protocol_win.cc index 503c00fef3..107ec1376a 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" @@ -144,8 +145,8 @@ bool SendToCrashHandlerServer(const std::wstring& pipe_name, bool SendPayloadToCrashHandlerServer( const std::wstring& pipe_name, const ClientToServerMessage& message, - const void* payload, - uint32_t payload_size, + base::span head, + base::span tail, ServerToClientMessage* response) { // Retry CreateFile() in a loop (follows the logic in // SendToCrashHandlerServer). @@ -183,9 +184,17 @@ bool SendPayloadToCrashHandlerServer( return false; } - if (payload_size > 0 && - !WriteFile(pipe.get(), payload, static_cast(payload_size))) { - PLOG(ERROR) << "WriteFile (payload)"; + 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 90de5ea2bf..d9ae2911a8 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" @@ -44,16 +45,16 @@ bool SendToCrashHandlerServer(const std::wstring& pipe_name, //! //! \param[in] pipe_name The name of the pipe to connect to. //! \param[in] message The message header to send. -//! \param[in] payload The payload bytes to send after \a message. -//! \param[in] payload_size The size of \a payload. +//! \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 SendPayloadToCrashHandlerServer( const std::wstring& pipe_name, const ClientToServerMessage& message, - const void* payload, - uint32_t payload_size, + base::span head, + base::span tail, ServerToClientMessage* response); //! \brief Wraps CreateNamedPipe() to create a single named pipe instance. From e5917a1ab2f563cb411d24e305a6da5cfdb58589 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Fri, 24 Jul 2026 15:28:52 +0200 Subject: [PATCH 18/19] restore original removal behavior to fix integration tests --- handler/win/crash_report_exception_handler.cc | 28 ++++++++++++------- handler/win/crash_report_exception_handler.h | 2 +- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 01608f00bd..334e66b4a7 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -49,7 +49,7 @@ CrashReportExceptionHandler::CrashReportExceptionHandler( : database_(database), upload_thread_(upload_thread), process_annotations_(process_annotations), - startup_attachments_(attachments), + startup_attachments_(*attachments), screenshot_(screenshot), wait_for_upload_(wait_for_upload), crash_reporter_(crash_reporter), @@ -126,7 +126,7 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( { base::AutoLock scoped_lock(attachments_lock_); - std::vector all_attachments(*startup_attachments_); + std::vector all_attachments(startup_attachments_); all_attachments.insert(all_attachments.end(), user_attachments_.begin(), user_attachments_.end()); @@ -173,7 +173,7 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( { base::AutoLock scoped_lock(attachments_lock_); if (envelope.Initialize(*crash_envelope_)) { - std::vector attachments(*startup_attachments_); + std::vector attachments(startup_attachments_); attachments.insert(attachments.end(), user_attachments_.begin(), user_attachments_.end()); @@ -229,10 +229,10 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAdded( bool CrashReportExceptionHandler::HasStartupAttachment( const base::FilePath& attachment) const { - return std::find(startup_attachments_->begin(), - startup_attachments_->end(), + return std::find(startup_attachments_.begin(), + startup_attachments_.end(), attachment) != - startup_attachments_->end(); + startup_attachments_.end(); } bool CrashReportExceptionHandler::HasUserAttachment( @@ -298,13 +298,21 @@ void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentAppended( void CrashReportExceptionHandler::ExceptionHandlerServerAttachmentRemoved( const base::FilePath& attachment) { base::AutoLock scoped_lock(attachments_lock_); - auto it = std::find( + auto startup_it = std::find( + startup_attachments_.begin(), startup_attachments_.end(), attachment); + if (startup_it != startup_attachments_.end()) { + startup_attachments_.erase(startup_it); + return; + } + + auto user_it = std::find( user_attachments_.begin(), user_attachments_.end(), attachment); - if (it == user_attachments_.end()) { - LOG(WARNING) << "ignoring non-existent attachment " << attachment; + if (user_it != user_attachments_.end()) { + user_attachments_.erase(user_it); return; } - user_attachments_.erase(it); + + 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 d6f3a4989e..429507cb6e 100644 --- a/handler/win/crash_report_exception_handler.h +++ b/handler/win/crash_report_exception_handler.h @@ -103,7 +103,7 @@ class CrashReportExceptionHandler final CrashReportUploadThread* upload_thread_; // weak const std::map* process_annotations_; // weak base::Lock attachments_lock_; - const std::vector* startup_attachments_; // weak + std::vector startup_attachments_; std::vector user_attachments_; const base::FilePath* screenshot_; // weak const bool wait_for_upload_; From 28495ae2bdb10c18338928fb5db4cc223e015e31 Mon Sep 17 00:00:00 2001 From: J-P Nurmi Date: Fri, 24 Jul 2026 16:13:25 +0200 Subject: [PATCH 19/19] length checks --- util/win/exception_handler_server.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index ff1e117f6c..24d04bbb06 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -478,7 +478,8 @@ static void HandleAddAttachmentV2( 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; } @@ -509,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; } @@ -545,7 +547,8 @@ static bool ReadAttachment( const uint32_t payload_length_bytes = message.attachment_write.payload_length_bytes; - if (path_length_bytes == 0 || path_length_bytes > kMaxPathBytes || + 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;