From 34aef02cc725795de2882e3ea30b048568048e12 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 20 Aug 2015 11:50:19 -0400 Subject: [PATCH] =?UTF-8?q?ubsan:=20Don=E2=80=99t=20call=20v[0]=20on=20emp?= =?UTF-8?q?ty=20vectors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling std::vector<>::operator[]() with an out-of-range index argument is undefined behavior. In two cases, Crashpad used &v[0] in situations where it was known that the address would not be used. These calls were wrapped in conditions guarding against vector emptiness. While s[0] is valid on an empty string, in two cases, Crashpad used &s[0] as an argument to a system call that would be a no-op. These calls were wrapped in similar conditions to avoid the system call. The two uses of vector with undefined behavior were caught by the following tests in crashpad_snapshot_test with UndefinedBehaviorSanitizer: [ RUN ] CrashpadInfoClientOptions.OneModule /Users/mark/compilatorium/llvm.build/bin/../include/c++/v1/vector:1493:12: runtime error: reference binding to null pointer of type 'crashpad::process_types::section' [ OK ] CrashpadInfoClientOptions.OneModule (72 ms) [ RUN ] ProcessSnapshotMinidump.Empty /Users/mark/compilatorium/llvm.build/bin/../include/c++/v1/vector:1493:12: runtime error: reference binding to null pointer of type 'MINIDUMP_DIRECTORY' [ OK ] ProcessSnapshotMinidump.Empty (1 ms) The Crashpad codebase was audited by searching for resize() calls and analyzing how resized strings and vectors are used. TEST=* BUG= R=rsesek@chromium.org Review URL: https://codereview.chromium.org/1283243004 . --- snapshot/mac/mach_o_image_segment_reader.cc | 3 ++- snapshot/minidump/process_snapshot_minidump.cc | 3 ++- util/mac/xattr.cc | 16 +++++++++------- util/mach/child_port_handshake.cc | 4 ++-- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/snapshot/mac/mach_o_image_segment_reader.cc b/snapshot/mac/mach_o_image_segment_reader.cc index 5c74d80d..9b07e904 100644 --- a/snapshot/mac/mach_o_image_segment_reader.cc +++ b/snapshot/mac/mach_o_image_segment_reader.cc @@ -95,7 +95,8 @@ bool MachOImageSegmentReader::Initialize(ProcessReader* process_reader, } sections_.resize(segment_command_.nsects); - if (!process_types::section::ReadArrayInto( + if (!sections_.empty() && + !process_types::section::ReadArrayInto( process_reader, load_command_address + segment_command_.Size(), segment_command_.nsects, diff --git a/snapshot/minidump/process_snapshot_minidump.cc b/snapshot/minidump/process_snapshot_minidump.cc index fa87981b..5344118c 100644 --- a/snapshot/minidump/process_snapshot_minidump.cc +++ b/snapshot/minidump/process_snapshot_minidump.cc @@ -65,7 +65,8 @@ bool ProcessSnapshotMinidump::Initialize(FileReaderInterface* file_reader) { } stream_directory_.resize(header_.NumberOfStreams); - if (!file_reader_->ReadExactly( + if (!stream_directory_.empty() && + !file_reader_->ReadExactly( &stream_directory_[0], header_.NumberOfStreams * sizeof(stream_directory_[0]))) { return false; diff --git a/util/mac/xattr.cc b/util/mac/xattr.cc index 54a8a09a..cb72e06e 100644 --- a/util/mac/xattr.cc +++ b/util/mac/xattr.cc @@ -41,14 +41,16 @@ XattrStatus ReadXattr(const base::FilePath& file, // Resize the buffer and read into it. value->resize(buffer_size); - ssize_t bytes_read = getxattr(file.value().c_str(), name.data(), - &(*value)[0], value->size(), - 0, 0); - if (bytes_read < 0) { - PLOG(ERROR) << "getxattr " << name << " on file " << file.value(); - return XattrStatus::kOtherError; + if (!value->empty()) { + ssize_t bytes_read = getxattr(file.value().c_str(), name.data(), + &(*value)[0], value->size(), + 0, 0); + if (bytes_read < 0) { + PLOG(ERROR) << "getxattr " << name << " on file " << file.value(); + return XattrStatus::kOtherError; + } + DCHECK_EQ(bytes_read, buffer_size); } - DCHECK_EQ(bytes_read, buffer_size); return XattrStatus::kOK; } diff --git a/util/mach/child_port_handshake.cc b/util/mach/child_port_handshake.cc index eedcf47b..efe1ea8a 100644 --- a/util/mach/child_port_handshake.cc +++ b/util/mach/child_port_handshake.cc @@ -319,8 +319,8 @@ void ChildPortHandshake::RunClientInternal_ReadPipe(int pipe_read, DCHECK_LT(service_name_length, implicit_cast(BOOTSTRAP_MAX_NAME_LEN)); - if (service_name_length > 0) { - service_name->resize(service_name_length); + service_name->resize(service_name_length); + if (!service_name->empty()) { CheckedReadFile(pipe_read, &(*service_name)[0], service_name_length); } }