From 981d4189aa28473edf681b8d4ac1050a78a39f3c Mon Sep 17 00:00:00 2001 From: Arthur Wang Date: Thu, 9 May 2024 12:39:17 -0700 Subject: [PATCH] Replace std::unique_ptr with HeapArray Bug: crashpad: 326459659,326458942,326459376,326459390,326459417,326458979,326459333,326459016,326458338,326458738,326459156,326459512,326458694 Change-Id: I04724530cbef50a8d3c18f306d16c0bbf3b0815b Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5512394 Reviewed-by: Mark Mentovai Commit-Queue: Arthur Wang --- .../cros_crash_report_exception_handler.cc | 9 ++- .../cros_crash_report_exception_handler.h | 2 + snapshot/capture_memory.cc | 10 +-- snapshot/memory_snapshot_generic.h | 7 +- snapshot/win/pe_image_reader.cc | 12 ++-- tools/tool_support.cc | 6 +- util/net/http_body_gzip_test.cc | 26 +++---- util/net/http_body_test_util.cc | 9 ++- util/process/process_memory_mac_test.cc | 8 +-- util/process/process_memory_test.cc | 72 +++++++++---------- util/win/exception_handler_server.cc | 11 +-- util/win/module_version.cc | 12 ++-- 12 files changed, 91 insertions(+), 93 deletions(-) diff --git a/handler/linux/cros_crash_report_exception_handler.cc b/handler/linux/cros_crash_report_exception_handler.cc index bfccd1f2..25637408 100644 --- a/handler/linux/cros_crash_report_exception_handler.cc +++ b/handler/linux/cros_crash_report_exception_handler.cc @@ -44,14 +44,13 @@ const std::string GetProcessNameFromPid(pid_t pid) { // Symlink to process binary is at /proc/###/exe. std::string link_path = "/proc/" + std::to_string(pid) + "/exe"; - constexpr int kMaxSize = 4096; - std::unique_ptr buf(new char[kMaxSize]); - ssize_t size = readlink(link_path.c_str(), buf.get(), kMaxSize); - std::string result; + std::string result(4096, '\0'); + ssize_t size = readlink(link_path.c_str(), result.data(), result.size()); if (size < 0) { PLOG(ERROR) << "Failed to readlink " << link_path; + result.clear(); } else { - result.assign(buf.get(), size); + result.resize(size); size_t last_slash_pos = result.rfind('/'); if (last_slash_pos != std::string::npos) { result = result.substr(last_slash_pos + 1); diff --git a/handler/linux/cros_crash_report_exception_handler.h b/handler/linux/cros_crash_report_exception_handler.h index 864de93c..0ac0c29f 100644 --- a/handler/linux/cros_crash_report_exception_handler.h +++ b/handler/linux/cros_crash_report_exception_handler.h @@ -15,6 +15,8 @@ #ifndef CRASHPAD_HANDLER_LINUX_CROS_CRASH_REPORT_EXCEPTION_HANDLER_H_ #define CRASHPAD_HANDLER_LINUX_CROS_CRASH_REPORT_EXCEPTION_HANDLER_H_ +#include + #include #include diff --git a/snapshot/capture_memory.cc b/snapshot/capture_memory.cc index c1c6fba5..fea93473 100644 --- a/snapshot/capture_memory.cc +++ b/snapshot/capture_memory.cc @@ -22,8 +22,8 @@ #include #include -#include +#include "base/containers/heap_array.h" #include "base/logging.h" #include "snapshot/memory_snapshot.h" @@ -140,16 +140,16 @@ void CaptureMemory::PointedToByMemoryRange(const MemorySnapshot& memory, return; } - std::unique_ptr buffer(new uint8_t[memory.Size()]); - if (!delegate->ReadMemory(memory.Address(), memory.Size(), buffer.get())) { + auto buffer = base::HeapArray::Uninit(memory.Size()); + if (!delegate->ReadMemory(memory.Address(), memory.Size(), buffer.data())) { LOG(ERROR) << "ReadMemory"; return; } if (delegate->Is64Bit()) - CaptureAtPointersInRange(buffer.get(), memory.Size(), delegate); + CaptureAtPointersInRange(buffer.data(), buffer.size(), delegate); else - CaptureAtPointersInRange(buffer.get(), memory.Size(), delegate); + CaptureAtPointersInRange(buffer.data(), buffer.size(), delegate); } } // namespace internal diff --git a/snapshot/memory_snapshot_generic.h b/snapshot/memory_snapshot_generic.h index 0187ada6..dd03bdbb 100644 --- a/snapshot/memory_snapshot_generic.h +++ b/snapshot/memory_snapshot_generic.h @@ -18,6 +18,7 @@ #include #include +#include "base/containers/heap_array.h" #include "base/logging.h" #include "base/numerics/safe_math.h" #include "snapshot/memory_snapshot.h" @@ -79,11 +80,11 @@ class MemorySnapshotGeneric final : public MemorySnapshot { return delegate->MemorySnapshotDelegateRead(nullptr, size_); } - std::unique_ptr buffer(new uint8_t[size_]); - if (!process_memory_->Read(address_, size_, buffer.get())) { + auto buffer = base::HeapArray::Uninit(size_); + if (!process_memory_->Read(address_, buffer.size(), buffer.data())) { return false; } - return delegate->MemorySnapshotDelegateRead(buffer.get(), size_); + return delegate->MemorySnapshotDelegateRead(buffer.data(), buffer.size()); } const MemorySnapshot* MergeWithOtherSnapshot( diff --git a/snapshot/win/pe_image_reader.cc b/snapshot/win/pe_image_reader.cc index e86c06e8..5ca53f2f 100644 --- a/snapshot/win/pe_image_reader.cc +++ b/snapshot/win/pe_image_reader.cc @@ -19,8 +19,8 @@ #include #include -#include +#include "base/containers/heap_array.h" #include "base/logging.h" #include "base/strings/stringprintf.h" #include "client/crashpad_info.h" @@ -183,17 +183,17 @@ bool PEImageReader::DebugDirectoryInformation(UUID* uuid, continue; } - std::unique_ptr data(new char[debug_directory.SizeOfData]); + auto data = base::HeapArray::Uninit(debug_directory.SizeOfData); if (!module_subrange_reader_.ReadMemory( Address() + debug_directory.AddressOfRawData, - debug_directory.SizeOfData, - data.get())) { + data.size(), + data.data())) { LOG(WARNING) << "could not read debug directory from " << module_subrange_reader_.name(); return false; } - if (*reinterpret_cast(data.get()) != + if (*reinterpret_cast(data.data()) != CodeViewRecordPDB70::kSignature) { LOG(WARNING) << "encountered non-7.0 CodeView debug record in " << module_subrange_reader_.name(); @@ -201,7 +201,7 @@ bool PEImageReader::DebugDirectoryInformation(UUID* uuid, } CodeViewRecordPDB70* codeview = - reinterpret_cast(data.get()); + reinterpret_cast(data.data()); *uuid = codeview->uuid; *age = codeview->age; // This is a NUL-terminated string encoded in the codepage of the system diff --git a/tools/tool_support.cc b/tools/tool_support.cc index 61d4bf85..cd83366a 100644 --- a/tools/tool_support.cc +++ b/tools/tool_support.cc @@ -16,9 +16,9 @@ #include -#include #include +#include "base/containers/heap_array.h" #include "base/strings/string_piece.h" #include "base/strings/utf_string_conversions.h" #include "build/build_config.h" @@ -77,7 +77,7 @@ void ToolSupport::UsageHint(const std::string& me, const char* hint) { // static int ToolSupport::Wmain(int argc, wchar_t* argv[], int (*entry)(int, char* [])) { - std::unique_ptr argv_as_utf8(new char*[argc + 1]); + auto argv_as_utf8 = base::HeapArray::Uninit(argc + 1); std::vector storage; storage.reserve(argc); for (int i = 0; i < argc; ++i) { @@ -85,7 +85,7 @@ int ToolSupport::Wmain(int argc, wchar_t* argv[], int (*entry)(int, char* [])) { argv_as_utf8[i] = &storage[i][0]; } argv_as_utf8[argc] = nullptr; - return entry(argc, argv_as_utf8.get()); + return entry(argc, argv_as_utf8.data()); } #endif // BUILDFLAG(IS_WIN) diff --git a/util/net/http_body_gzip_test.cc b/util/net/http_body_gzip_test.cc index a9438e0c..6fed433a 100644 --- a/util/net/http_body_gzip_test.cc +++ b/util/net/http_body_gzip_test.cc @@ -21,8 +21,9 @@ #include #include -#include "base/rand_util.h" +#include "base/containers/heap_array.h" #include "base/numerics/safe_conversions.h" +#include "base/rand_util.h" #include "gtest/gtest.h" #include "third_party/zlib/zlib_crashpad.h" #include "util/misc/zlib.h" @@ -54,17 +55,16 @@ void GzipInflate(const std::string& compressed, decompressed->clear(); // There’s got to be at least a small buffer. - buf_size = std::max(buf_size, static_cast(1)); - - std::unique_ptr buf(new uint8_t[buf_size]); + auto buf = base::HeapArray::Uninit( + std::max(buf_size, static_cast(1))); z_stream zlib = {}; zlib.zalloc = Z_NULL; zlib.zfree = Z_NULL; zlib.opaque = Z_NULL; zlib.next_in = reinterpret_cast(const_cast(&compressed[0])); zlib.avail_in = base::checked_cast(compressed.size()); - zlib.next_out = buf.get(); - zlib.avail_out = base::checked_cast(buf_size); + zlib.next_out = buf.data(); + zlib.avail_out = base::checked_cast(buf.size()); int zr = inflateInit2(&zlib, ZlibWindowBitsWithGzipWrapper(0)); ASSERT_EQ(zr, Z_OK) << "inflateInit2: " << ZlibErrorString(zr); @@ -73,9 +73,9 @@ void GzipInflate(const std::string& compressed, zr = inflate(&zlib, Z_FINISH); ASSERT_EQ(zr, Z_STREAM_END) << "inflate: " << ZlibErrorString(zr); - ASSERT_LE(zlib.avail_out, buf_size); - decompressed->assign(reinterpret_cast(buf.get()), - buf_size - zlib.avail_out); + ASSERT_LE(zlib.avail_out, buf.size()); + decompressed->assign(reinterpret_cast(buf.data()), + buf.size() - zlib.avail_out); } void TestGzipDeflateInflate(const std::string& string) { @@ -94,17 +94,17 @@ void TestGzipDeflateInflate(const std::string& string) { size_t buf_size = string.size() + kGzipHeaderSize + (string.empty() ? 2 : (((string.size() + 16383) / 16384) * 5)); - std::unique_ptr buf(new uint8_t[buf_size]); + auto buf = base::HeapArray::Uninit(buf_size); FileOperationResult compressed_bytes = - gzip_stream.GetBytesBuffer(buf.get(), buf_size); + gzip_stream.GetBytesBuffer(buf.data(), buf.size()); ASSERT_NE(compressed_bytes, -1); - ASSERT_LE(static_cast(compressed_bytes), buf_size); + ASSERT_LE(static_cast(compressed_bytes), buf.size()); // Make sure that the stream is really at EOF. uint8_t eof_buf[16]; ASSERT_EQ(gzip_stream.GetBytesBuffer(eof_buf, sizeof(eof_buf)), 0); - std::string compressed(reinterpret_cast(buf.get()), compressed_bytes); + std::string compressed(reinterpret_cast(buf.data()), compressed_bytes); ASSERT_GE(compressed.size(), kGzipHeaderSize); EXPECT_EQ(compressed[0], '\37'); diff --git a/util/net/http_body_test_util.cc b/util/net/http_body_test_util.cc index b011f4e3..960eb62a 100644 --- a/util/net/http_body_test_util.cc +++ b/util/net/http_body_test_util.cc @@ -16,8 +16,7 @@ #include -#include - +#include "base/containers/heap_array.h" #include "gtest/gtest.h" #include "util/file/file_io.h" #include "util/net/http_body.h" @@ -30,17 +29,17 @@ std::string ReadStreamToString(HTTPBodyStream* stream) { } std::string ReadStreamToString(HTTPBodyStream* stream, size_t buffer_size) { - std::unique_ptr buf(new uint8_t[buffer_size]); + auto buf = base::HeapArray::Uninit(buffer_size); std::string result; FileOperationResult bytes_read; - while ((bytes_read = stream->GetBytesBuffer(buf.get(), buffer_size)) != 0) { + while ((bytes_read = stream->GetBytesBuffer(buf.data(), buf.size())) != 0) { if (bytes_read < 0) { ADD_FAILURE() << "Failed to read from stream: " << bytes_read; return std::string(); } - result.append(reinterpret_cast(buf.get()), bytes_read); + result.append(reinterpret_cast(buf.data()), bytes_read); } return result; diff --git a/util/process/process_memory_mac_test.cc b/util/process/process_memory_mac_test.cc index 296b536d..08e897b7 100644 --- a/util/process/process_memory_mac_test.cc +++ b/util/process/process_memory_mac_test.cc @@ -23,6 +23,7 @@ #include "base/apple/scoped_mach_port.h" #include "base/apple/scoped_mach_vm.h" +#include "base/containers/heap_array.h" #include "gtest/gtest.h" #include "test/mac/mach_errors.h" #include "util/misc/from_pointer_cast.h" @@ -259,13 +260,12 @@ TEST(ProcessMemoryMac, MappedMemoryDeallocates) { // This is the same but with a big buffer that’s definitely larger than a // single page. This makes sure that the whole mapped region winds up being // deallocated. - const size_t kBigSize = 4 * PAGE_SIZE; - std::unique_ptr big_buffer(new char[kBigSize]); + auto big_buffer = base::HeapArray::Uninit(4 * PAGE_SIZE); test_address = FromPointerCast(&big_buffer[0]); - ASSERT_TRUE((mapped = memory.ReadMapped(test_address, kBigSize))); + ASSERT_TRUE((mapped = memory.ReadMapped(test_address, big_buffer.size()))); mapped_address = reinterpret_cast(mapped->data()); - vm_address_t mapped_last_address = mapped_address + kBigSize - 1; + vm_address_t mapped_last_address = mapped_address + big_buffer.size() - 1; EXPECT_TRUE(IsAddressMapped(mapped_address)); EXPECT_TRUE(IsAddressMapped(mapped_address + PAGE_SIZE)); EXPECT_TRUE(IsAddressMapped(mapped_last_address)); diff --git a/util/process/process_memory_test.cc b/util/process/process_memory_test.cc index 11558e40..07b42535 100644 --- a/util/process/process_memory_test.cc +++ b/util/process/process_memory_test.cc @@ -16,8 +16,7 @@ #include -#include - +#include "base/containers/heap_array.h" #include "base/memory/page_size.h" #include "build/build_config.h" #include "gtest/gtest.h" @@ -105,22 +104,20 @@ class MultiprocessAdaptor : public MultiprocessExec { }; #endif // BUILDFLAG(IS_APPLE) -void DoChildReadTestSetup(size_t* region_size, - std::unique_ptr* region) { - *region_size = 4 * base::GetPageSize(); - region->reset(new char[*region_size]); - for (size_t index = 0; index < *region_size; ++index) { - (*region)[index] = static_cast(index % 256); +base::HeapArray DoChildReadTestSetup() { + auto region = base::HeapArray::Uninit(4 * base::GetPageSize()); + for (size_t index = 0; index < region.size(); ++index) { + region[index] = static_cast(index % 256); } + return region; } CRASHPAD_CHILD_TEST_MAIN(ReadTestChild) { - size_t region_size; - std::unique_ptr region; - DoChildReadTestSetup(®ion_size, ®ion); + auto region = DoChildReadTestSetup(); + auto region_size = region.size(); FileHandle out = MultiprocessAdaptor::OutputHandle(); CheckedWriteFile(out, ®ion_size, sizeof(region_size)); - VMAddress address = FromPointerCast(region.get()); + VMAddress address = FromPointerCast(region.data()); CheckedWriteFile(out, &address, sizeof(address)); CheckedReadFileAtEOF(MultiprocessAdaptor::InputHandle()); return 0; @@ -136,12 +133,10 @@ class ReadTest : public MultiprocessAdaptor { ReadTest& operator=(const ReadTest&) = delete; void RunAgainstSelf() { - size_t region_size; - std::unique_ptr region; - DoChildReadTestSetup(®ion_size, ®ion); + auto region = DoChildReadTestSetup(); DoTest(GetSelfProcess(), - region_size, - FromPointerCast(region.get())); + region.size(), + FromPointerCast(region.data())); } void RunAgainstChild() { Run(); } @@ -167,50 +162,50 @@ class ReadTest : public MultiprocessAdaptor { #endif // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_LINUX) || // BUILDFLAG(IS_CHROMEOS) - std::unique_ptr result(new char[region_size]); + auto result = base::HeapArray::Uninit(region_size); // Ensure that the entire region can be read. - ASSERT_TRUE(memory.Read(address, region_size, result.get())); - for (size_t i = 0; i < region_size; ++i) { + ASSERT_TRUE(memory.Read(address, result.size(), result.data())); + for (size_t i = 0; i < result.size(); ++i) { EXPECT_EQ(result[i], static_cast(i % 256)); } // Ensure that a read of length 0 succeeds and doesn’t touch the result. - memset(result.get(), '\0', region_size); - ASSERT_TRUE(memory.Read(address, 0, result.get())); - for (size_t i = 0; i < region_size; ++i) { + memset(result.data(), '\0', result.size()); + ASSERT_TRUE(memory.Read(address, 0, result.data())); + for (size_t i = 0; i < result.size(); ++i) { EXPECT_EQ(result[i], 0); } // Ensure that a read starting at an unaligned address works. - ASSERT_TRUE(memory.Read(address + 1, region_size - 1, result.get())); - for (size_t i = 0; i < region_size - 1; ++i) { + ASSERT_TRUE(memory.Read(address + 1, result.size() - 1, result.data())); + for (size_t i = 0; i < result.size() - 1; ++i) { EXPECT_EQ(result[i], static_cast((i + 1) % 256)); } // Ensure that a read ending at an unaligned address works. - ASSERT_TRUE(memory.Read(address, region_size - 1, result.get())); - for (size_t i = 0; i < region_size - 1; ++i) { + ASSERT_TRUE(memory.Read(address, result.size() - 1, result.data())); + for (size_t i = 0; i < result.size() - 1; ++i) { EXPECT_EQ(result[i], static_cast(i % 256)); } // Ensure that a read starting and ending at unaligned addresses works. - ASSERT_TRUE(memory.Read(address + 1, region_size - 2, result.get())); - for (size_t i = 0; i < region_size - 2; ++i) { + ASSERT_TRUE(memory.Read(address + 1, result.size() - 2, result.data())); + for (size_t i = 0; i < result.size() - 2; ++i) { EXPECT_EQ(result[i], static_cast((i + 1) % 256)); } // Ensure that a read of exactly one page works. size_t page_size = base::GetPageSize(); - ASSERT_GE(region_size, page_size + page_size); - ASSERT_TRUE(memory.Read(address + page_size, page_size, result.get())); + ASSERT_GE(result.size(), page_size + page_size); + ASSERT_TRUE(memory.Read(address + page_size, page_size, result.data())); for (size_t i = 0; i < page_size; ++i) { EXPECT_EQ(result[i], static_cast((i + page_size) % 256)); } // Ensure that reading exactly a single byte works. result[1] = 'J'; - ASSERT_TRUE(memory.Read(address + 2, 1, result.get())); + ASSERT_TRUE(memory.Read(address + 2, 1, result.data())); EXPECT_EQ(result[0], 2); EXPECT_EQ(result[1], 'J'); } @@ -437,14 +432,13 @@ class ReadUnmappedTest : public MultiprocessAdaptor { VMAddress page_addr1 = address; VMAddress page_addr2 = page_addr1 + base::GetPageSize(); - std::unique_ptr result(new char[base::GetPageSize() * 2]); - EXPECT_TRUE(memory.Read(page_addr1, base::GetPageSize(), result.get())); - EXPECT_TRUE(memory.Read(page_addr2 - 1, 1, result.get())); + auto result = base::HeapArray::Uninit(base::GetPageSize() * 2); + EXPECT_TRUE(memory.Read(page_addr1, base::GetPageSize(), result.data())); + EXPECT_TRUE(memory.Read(page_addr2 - 1, 1, result.data())); - EXPECT_FALSE( - memory.Read(page_addr1, base::GetPageSize() * 2, result.get())); - EXPECT_FALSE(memory.Read(page_addr2, base::GetPageSize(), result.get())); - EXPECT_FALSE(memory.Read(page_addr2 - 1, 2, result.get())); + EXPECT_FALSE(memory.Read(page_addr1, result.size(), result.data())); + EXPECT_FALSE(memory.Read(page_addr2, base::GetPageSize(), result.data())); + EXPECT_FALSE(memory.Read(page_addr2 - 1, 2, result.data())); } }; diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index e641c7fb..8af53a62 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -22,6 +22,7 @@ #include #include "base/check.h" +#include "base/containers/heap_array.h" #include "base/logging.h" #include "base/numerics/safe_conversions.h" #include "base/rand_util.h" @@ -270,16 +271,16 @@ void ExceptionHandlerServer::InitializeWithInheritedDataForInitialClient( first_pipe_instance_.reset(initial_client_data.first_pipe_instance()); // TODO(scottmg): Vista+. Might need to pass through or possibly find an Nt*. - size_t bytes = sizeof(wchar_t) * _MAX_PATH + sizeof(FILE_NAME_INFO); - std::unique_ptr data(new uint8_t[bytes]); + auto data = base::HeapArray::Uninit(sizeof(wchar_t) * _MAX_PATH + + sizeof(FILE_NAME_INFO)); if (!GetFileInformationByHandleEx(first_pipe_instance_.get(), FileNameInfo, - data.get(), - static_cast(bytes))) { + data.data(), + static_cast(data.size()))) { PLOG(FATAL) << "GetFileInformationByHandleEx"; } FILE_NAME_INFO* file_name_info = - reinterpret_cast(data.get()); + reinterpret_cast(data.data()); pipe_name_ = L"\\\\.\\pipe" + std::wstring(file_name_info->FileName, file_name_info->FileNameLength / diff --git a/util/win/module_version.cc b/util/win/module_version.cc index ca7aef83..e0623669 100644 --- a/util/win/module_version.cc +++ b/util/win/module_version.cc @@ -17,8 +17,7 @@ #include #include -#include - +#include "base/containers/heap_array.h" #include "base/logging.h" #include "base/strings/utf_string_conversions.h" @@ -33,15 +32,18 @@ bool GetModuleVersionAndType(const base::FilePath& path, return false; } - std::unique_ptr data(new uint8_t[size]); - if (!GetFileVersionInfo(path.value().c_str(), 0, size, data.get())) { + auto data = base::HeapArray::Uninit(size); + if (!GetFileVersionInfo(path.value().c_str(), + 0, + static_cast(data.size()), + data.data())) { PLOG(WARNING) << "GetFileVersionInfo: " << base::WideToUTF8(path.value()); return false; } VS_FIXEDFILEINFO* fixed_file_info; UINT ffi_size; - if (!VerQueryValue(data.get(), + if (!VerQueryValue(data.data(), L"\\", reinterpret_cast(&fixed_file_info), &ffi_size)) {