From 15103742e06e415cf8b88d1726dc321108fed4be Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 28 Apr 2017 10:08:35 -0400 Subject: [PATCH] Use FromPointerCast<>() in many places where it makes sense I opted to leave casts to types that were definitely the same size alone. reinterpret_cast(pointer) and reinterpret_cast(pointer) should always be safe, for example. Casts to other integral types have been replaced with FromPointerCast<>(), which does zero-extension or sign-extension based on the target type. To make it possible to use FromPointerCast<>() with some use sites that were already using checked_cast<>(), FromPointerCast<>() now uses check_cast<>() when converting to a narrower type. Test: crashpad_util_test FromPointerCast*, others Change-Id: I4a71b4aa2d87f545c75524290a702f5f3138d675 Reviewed-on: https://chromium-review.googlesource.com/489701 Reviewed-by: Scott Graham --- client/crashpad_client_win.cc | 29 ++++---- client/crashpad_info.cc | 8 +-- client/simple_address_range_bag.h | 12 ++-- snapshot/api/module_annotations_win.cc | 3 +- snapshot/mac/mach_o_image_reader_test.cc | 13 ++-- snapshot/mac/process_reader_test.cc | 20 +++--- snapshot/mac/process_types_test.cc | 3 +- .../crashpad_snapshot_test_crashing_child.cc | 3 +- ...pad_snapshot_test_dump_without_crashing.cc | 3 +- snapshot/win/pe_image_reader.cc | 5 +- snapshot/win/pe_image_reader_test.cc | 5 +- snapshot/win/process_reader_win_test.cc | 9 ++- snapshot/win/process_snapshot_win.cc | 3 +- util/mach/task_memory_test.cc | 11 +-- util/misc/from_pointer_cast.h | 52 +++++++------- util/misc/from_pointer_cast_test.cc | 71 +++++++++++++++++++ util/win/handle.cc | 3 +- util/win/process_info.cc | 3 +- util/win/process_info_test.cc | 5 +- 19 files changed, 173 insertions(+), 88 deletions(-) diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 1a183c8a..487b39ab 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -30,6 +30,7 @@ #include "base/strings/utf_string_conversions.h" #include "base/synchronization/lock.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/random_string.h" #include "util/win/address_types.h" #include "util/win/capture_context.h" @@ -156,7 +157,7 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { // signal the crash handler. g_crash_exception_information.thread_id = GetCurrentThreadId(); g_crash_exception_information.exception_pointers = - reinterpret_cast(exception_pointers); + FromPointerCast(exception_pointers); // Now signal the crash server, which will take a dump and then terminate us // when it's complete. @@ -390,9 +391,9 @@ bool StartHandlerProcess( g_non_crash_dump_done, data->ipc_pipe_handle.get(), this_process.get(), - reinterpret_cast(&g_crash_exception_information), - reinterpret_cast(&g_non_crash_exception_information), - reinterpret_cast(&g_critical_section_with_debug_info)); + FromPointerCast(&g_crash_exception_information), + FromPointerCast(&g_non_crash_exception_information), + FromPointerCast(&g_critical_section_with_debug_info)); AppendCommandLineArgument( base::UTF8ToUTF16(std::string("--initial-client-data=") + initial_client_data.StringRepresentation()), @@ -655,14 +656,14 @@ bool CrashpadClient::SetHandlerIPCPipe(const std::wstring& ipc_pipe) { message.registration.version = RegistrationRequest::kMessageVersion; message.registration.client_process_id = GetCurrentProcessId(); message.registration.crash_exception_information = - reinterpret_cast(&g_crash_exception_information); + FromPointerCast(&g_crash_exception_information); message.registration.non_crash_exception_information = - reinterpret_cast(&g_non_crash_exception_information); + FromPointerCast(&g_non_crash_exception_information); CommonInProcessInitialization(); message.registration.critical_section_address = - reinterpret_cast(&g_critical_section_with_debug_info); + FromPointerCast(&g_critical_section_with_debug_info); ServerToClientMessage response = {}; @@ -765,7 +766,7 @@ void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) { g_non_crash_exception_information.thread_id = GetCurrentThreadId(); g_non_crash_exception_information.exception_pointers = - reinterpret_cast(&exception_pointers); + FromPointerCast(&exception_pointers); bool set_event_result = !!SetEvent(g_signal_non_crash_dump); PLOG_IF(ERROR, !set_event_result) << "SetEvent"; @@ -830,11 +831,11 @@ bool CrashpadClient::DumpAndCrashTargetProcess(HANDLE process, const size_t kInjectBufferSize = 4 * 1024; WinVMAddress inject_memory = - reinterpret_cast(VirtualAllocEx(process, - nullptr, - kInjectBufferSize, - MEM_RESERVE | MEM_COMMIT, - PAGE_READWRITE)); + FromPointerCast(VirtualAllocEx(process, + nullptr, + kInjectBufferSize, + MEM_RESERVE | MEM_COMMIT, + PAGE_READWRITE)); if (!inject_memory) { PLOG(ERROR) << "VirtualAllocEx"; return false; @@ -844,7 +845,7 @@ bool CrashpadClient::DumpAndCrashTargetProcess(HANDLE process, // loaded at the same address in our process as the target, and just look up // its address here. WinVMAddress raise_exception_address = - reinterpret_cast(&RaiseException); + FromPointerCast(&RaiseException); WinVMAddress code_entry_point = 0; std::vector data_to_write; diff --git a/client/crashpad_info.cc b/client/crashpad_info.cc index e517f7b1..b7ba5490 100644 --- a/client/crashpad_info.cc +++ b/client/crashpad_info.cc @@ -15,6 +15,7 @@ #include "client/crashpad_info.h" #include "util/misc/address_sanitizer.h" +#include "util/misc/from_pointer_cast.h" #include "util/stdlib/cxx.h" #if defined(OS_MACOSX) @@ -131,11 +132,10 @@ void CrashpadInfo::AddUserDataMinidumpStream(uint32_t stream_type, const void* data, size_t size) { auto to_be_added = new internal::UserDataMinidumpStreamListEntry(); - to_be_added->next = base::checked_cast( - reinterpret_cast(user_data_minidump_stream_head_)); + to_be_added->next = + FromPointerCast(user_data_minidump_stream_head_); to_be_added->stream_type = stream_type; - to_be_added->base_address = - base::checked_cast(reinterpret_cast(data)); + to_be_added->base_address = FromPointerCast(data); to_be_added->size = base::checked_cast(size); user_data_minidump_stream_head_ = to_be_added; } diff --git a/client/simple_address_range_bag.h b/client/simple_address_range_bag.h index 336a748d..a09ae6e8 100644 --- a/client/simple_address_range_bag.h +++ b/client/simple_address_range_bag.h @@ -19,6 +19,8 @@ #include "base/logging.h" #include "base/macros.h" +#include "base/numerics/safe_conversions.h" +#include "util/misc/from_pointer_cast.h" #include "util/numeric/checked_range.h" namespace crashpad { @@ -138,9 +140,8 @@ class TSimpleAddressRangeBag { bool Insert(void* base, size_t size) { DCHECK(base != nullptr); DCHECK_NE(0u, size); - return Insert(CheckedRange( - base::checked_cast(reinterpret_cast(base)), - base::checked_cast(size))); + return Insert(CheckedRange(FromPointerCast(base), + base::checked_cast(size))); } //! \brief Removes the given range from the bag. @@ -175,9 +176,8 @@ class TSimpleAddressRangeBag { bool Remove(void* base, size_t size) { DCHECK(base != nullptr); DCHECK_NE(0u, size); - return Remove(CheckedRange( - base::checked_cast(reinterpret_cast(base)), - base::checked_cast(size))); + return Remove(CheckedRange(FromPointerCast(base), + base::checked_cast(size))); } diff --git a/snapshot/api/module_annotations_win.cc b/snapshot/api/module_annotations_win.cc index 8f444fa5..fd468703 100644 --- a/snapshot/api/module_annotations_win.cc +++ b/snapshot/api/module_annotations_win.cc @@ -17,6 +17,7 @@ #include "snapshot/win/pe_image_annotations_reader.h" #include "snapshot/win/pe_image_reader.h" #include "snapshot/win/process_reader_win.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/get_module_information.h" namespace crashpad { @@ -38,7 +39,7 @@ bool ReadModuleAnnotations(HANDLE process, PEImageReader image_reader; if (!image_reader.Initialize( &process_reader, - reinterpret_cast(module_info.lpBaseOfDll), + FromPointerCast(module_info.lpBaseOfDll), module_info.SizeOfImage, "")) return false; diff --git a/snapshot/mac/mach_o_image_reader_test.cc b/snapshot/mac/mach_o_image_reader_test.cc index cccf24fa..6c545bc1 100644 --- a/snapshot/mac/mach_o_image_reader_test.cc +++ b/snapshot/mac/mach_o_image_reader_test.cc @@ -32,6 +32,7 @@ #include "snapshot/mac/process_reader.h" #include "snapshot/mac/process_types.h" #include "test/mac/dyld.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/implicit_cast.h" #include "util/misc/uuid.h" @@ -133,7 +134,7 @@ void ExpectSegmentCommand(const SegmentCommand* expect_segment, const uint8_t* expect_segment_data = getsegmentdata( expect_image, segment_name.c_str(), &expect_segment_size); mach_vm_address_t expect_segment_address = - reinterpret_cast(expect_segment_data); + FromPointerCast(expect_segment_data); EXPECT_EQ(actual_segment->Address(), expect_segment_address); EXPECT_EQ(actual_segment->vmsize(), expect_segment_size); EXPECT_EQ(actual_segment->Size(), actual_segment->vmsize()); @@ -191,7 +192,7 @@ void ExpectSegmentCommand(const SegmentCommand* expect_segment, section_name.c_str(), &expect_section_size); mach_vm_address_t expect_section_address = - reinterpret_cast(expect_section_data); + FromPointerCast(expect_section_data); EXPECT_EQ(actual_section_address, expect_section_address); EXPECT_EQ(actual_section->size, expect_section_size); } else { @@ -501,7 +502,7 @@ TEST(MachOImageReader, Self_MainExecutable) { reinterpret_cast(dlsym(RTLD_MAIN_ONLY, MH_EXECUTE_SYM)); ASSERT_NE(mh_execute_header, nullptr); mach_vm_address_t mh_execute_header_address = - reinterpret_cast(mh_execute_header); + FromPointerCast(mh_execute_header); MachOImageReader image_reader; ASSERT_TRUE(image_reader.Initialize( @@ -547,7 +548,7 @@ TEST(MachOImageReader, Self_DyldImages) { const MachHeader* mach_header = reinterpret_cast(_dyld_get_image_header(index)); mach_vm_address_t image_address = - reinterpret_cast(mach_header); + FromPointerCast(mach_header); MachOImageReader image_reader; ASSERT_TRUE( @@ -588,7 +589,7 @@ TEST(MachOImageReader, Self_DyldImages) { const MachHeader* mach_header = reinterpret_cast( dyld_image_infos->dyldImageLoadAddress); mach_vm_address_t image_address = - reinterpret_cast(mach_header); + FromPointerCast(mach_header); MachOImageReader image_reader; ASSERT_TRUE( @@ -619,7 +620,7 @@ TEST(MachOImageReader, Self_DyldImages) { const MachHeader* mach_header = reinterpret_cast(dyld_image->imageLoadAddress); mach_vm_address_t image_address = - reinterpret_cast(mach_header); + FromPointerCast(mach_header); MachOImageReader image_reader; ASSERT_TRUE( diff --git a/snapshot/mac/process_reader_test.cc b/snapshot/mac/process_reader_test.cc index 26cb26d2..3554326c 100644 --- a/snapshot/mac/process_reader_test.cc +++ b/snapshot/mac/process_reader_test.cc @@ -40,6 +40,7 @@ #include "util/file/file_io.h" #include "util/mac/mac_util.h" #include "util/mach/mach_extensions.h" +#include "util/misc/from_pointer_cast.h" #include "util/stdlib/pointer_container.h" #include "util/synchronization/semaphore.h" @@ -74,7 +75,7 @@ TEST(ProcessReader, SelfBasic) { const char kTestMemory[] = "Some test memory"; char buffer[arraysize(kTestMemory)]; ASSERT_TRUE(process_reader.Memory()->Read( - reinterpret_cast(kTestMemory), + FromPointerCast(kTestMemory), sizeof(kTestMemory), &buffer)); EXPECT_STREQ(kTestMemory, buffer); @@ -115,8 +116,7 @@ class ProcessReaderChild final : public MachMultiprocess { void MachMultiprocessChild() override { FileHandle write_handle = WritePipeHandle(); - mach_vm_address_t address = - reinterpret_cast(kTestMemory); + mach_vm_address_t address = FromPointerCast(kTestMemory); CheckedWriteFile(write_handle, &address, sizeof(address)); // Wait for the parent to signal that it’s OK to exit by closing its end of @@ -282,7 +282,7 @@ class TestThreadPool { ThreadInfo* thread_info = static_cast(argument); thread_info->stack_address = - reinterpret_cast(&thread_info); + FromPointerCast(&thread_info); thread_info->ready_semaphore.Signal(); thread_info->exit_semaphore.Wait(); @@ -391,7 +391,7 @@ TEST(ProcessReader, SelfSeveralThreads) { ThreadMap thread_map; const uint64_t self_thread_id = PthreadToThreadID(pthread_self()); TestThreadPool::ThreadExpectation expectation; - expectation.stack_address = reinterpret_cast(&thread_map); + expectation.stack_address = FromPointerCast(&thread_map); expectation.suspend_count = 0; thread_map[self_thread_id] = expectation; for (size_t thread_index = 0; thread_index < kChildThreads; ++thread_index) { @@ -484,7 +484,7 @@ class ProcessReaderThreadedChild final : public MachMultiprocess { CheckedWriteFile(write_handle, &thread_id, sizeof(thread_id)); TestThreadPool::ThreadExpectation expectation; - expectation.stack_address = reinterpret_cast(&thread_id); + expectation.stack_address = FromPointerCast(&thread_id); expectation.suspend_count = 0; CheckedWriteFile(write_handle, @@ -667,7 +667,7 @@ TEST(ProcessReader, SelfModules) { ASSERT_TRUE(modules[index].reader); EXPECT_EQ( modules[index].reader->Address(), - reinterpret_cast(_dyld_get_image_header(index))); + FromPointerCast(_dyld_get_image_header(index))); if (index == 0) { // dyld didn’t load the main executable, so it couldn’t record its @@ -703,7 +703,7 @@ TEST(ProcessReader, SelfModules) { if (dyld_image_infos->version >= 2) { ASSERT_TRUE(modules[index].reader); EXPECT_EQ(modules[index].reader->Address(), - reinterpret_cast( + FromPointerCast( dyld_image_infos->dyldImageLoadAddress)); } } @@ -801,10 +801,10 @@ class ProcessReaderModulesChild final : public MachMultiprocess { if (index < dyld_image_count) { dyld_image_name = _dyld_get_image_name(index); dyld_image_address = - reinterpret_cast(_dyld_get_image_header(index)); + FromPointerCast(_dyld_get_image_header(index)); } else { dyld_image_name = kDyldPath; - dyld_image_address = reinterpret_cast( + dyld_image_address = FromPointerCast( dyld_image_infos->dyldImageLoadAddress); } diff --git a/snapshot/mac/process_types_test.cc b/snapshot/mac/process_types_test.cc index f9ff7d19..18595f02 100644 --- a/snapshot/mac/process_types_test.cc +++ b/snapshot/mac/process_types_test.cc @@ -27,6 +27,7 @@ #include "snapshot/mac/process_types/internal.h" #include "test/mac/dyld.h" #include "util/mac/mac_util.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/implicit_cast.h" namespace crashpad { @@ -81,7 +82,7 @@ TEST(ProcessTypes, DyldImagesSelf) { ASSERT_EQ(kr, KERN_SUCCESS); EXPECT_EQ(dyld_info.all_image_info_addr, - reinterpret_cast(self_image_infos)); + FromPointerCast(self_image_infos)); EXPECT_GT(dyld_info.all_image_info_size, 1u); // This field is only present in the OS X 10.7 SDK (at build time) and kernel diff --git a/snapshot/win/crashpad_snapshot_test_crashing_child.cc b/snapshot/win/crashpad_snapshot_test_crashing_child.cc index b0caa065..146c66a1 100644 --- a/snapshot/win/crashpad_snapshot_test_crashing_child.cc +++ b/snapshot/win/crashpad_snapshot_test_crashing_child.cc @@ -19,12 +19,13 @@ #include "base/logging.h" #include "client/crashpad_client.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/address_types.h" namespace { __declspec(noinline) crashpad::WinVMAddress CurrentAddress() { - return reinterpret_cast(_ReturnAddress()); + return crashpad::FromPointerCast(_ReturnAddress()); } } // namespace diff --git a/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc b/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc index a9fbdcf5..f55f503b 100644 --- a/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc +++ b/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc @@ -19,12 +19,13 @@ #include "client/crashpad_client.h" #include "client/simulate_crash.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/address_types.h" namespace { __declspec(noinline) crashpad::WinVMAddress CurrentAddress() { - return reinterpret_cast(_ReturnAddress()); + return crashpad::FromPointerCast(_ReturnAddress()); } } // namespace diff --git a/snapshot/win/pe_image_reader.cc b/snapshot/win/pe_image_reader.cc index 342021a6..824f525b 100644 --- a/snapshot/win/pe_image_reader.cc +++ b/snapshot/win/pe_image_reader.cc @@ -22,6 +22,7 @@ #include "base/logging.h" #include "client/crashpad_info.h" #include "snapshot/win/pe_image_resource_reader.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/pdb_structures.h" #include "util/win/process_structs.h" @@ -202,10 +203,8 @@ bool PEImageReader::VSFixedFileInfo( WinVMAddress address; WinVMSize size; - const uint16_t vs_file_info_type = static_cast( - reinterpret_cast(VS_FILE_INFO)); // RT_VERSION if (!resource_reader.FindResourceByID( - vs_file_info_type, + FromPointerCast(VS_FILE_INFO), // RT_VERSION VS_VERSION_INFO, MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL), &address, diff --git a/snapshot/win/pe_image_reader_test.cc b/snapshot/win/pe_image_reader_test.cc index 2468c140..041d72a2 100644 --- a/snapshot/win/pe_image_reader_test.cc +++ b/snapshot/win/pe_image_reader_test.cc @@ -22,6 +22,7 @@ #include "gtest/gtest.h" #include "snapshot/win/process_reader_win.h" #include "test/errors.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/get_module_information.h" #include "util/win/module_version.h" #include "util/win/process_info.h" @@ -44,7 +45,7 @@ TEST(PEImageReader, DebugDirectory) { << ErrorMessage("GetModuleInformation"); EXPECT_EQ(module_info.lpBaseOfDll, self); ASSERT_TRUE(pe_image_reader.Initialize(&process_reader, - reinterpret_cast(self), + FromPointerCast(self), module_info.SizeOfImage, "self")); UUID uuid; @@ -139,7 +140,7 @@ TEST(PEImageReader, VSFixedFileInfo_OneModule) { ProcessInfo::Module module; module.name = kModuleName; - module.dll_base = reinterpret_cast(module_info.lpBaseOfDll); + module.dll_base = FromPointerCast(module_info.lpBaseOfDll); module.size = module_info.SizeOfImage; TestVSFixedFileInfo(&process_reader, module, true); diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index 5b4b7d0a..87e72a94 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -19,6 +19,7 @@ #include "gtest/gtest.h" #include "test/win/win_multiprocess.h" +#include "util/misc/from_pointer_cast.h" #include "util/synchronization/semaphore.h" #include "util/thread/thread.h" #include "util/win/scoped_process_suspend.h" @@ -42,10 +43,8 @@ TEST(ProcessReaderWin, SelfBasic) { const char kTestMemory[] = "Some test memory"; char buffer[arraysize(kTestMemory)]; - ASSERT_TRUE( - process_reader.ReadMemory(reinterpret_cast(kTestMemory), - sizeof(kTestMemory), - &buffer)); + ASSERT_TRUE(process_reader.ReadMemory( + reinterpret_cast(kTestMemory), sizeof(kTestMemory), &buffer)); EXPECT_STREQ(kTestMemory, buffer); } @@ -78,7 +77,7 @@ class ProcessReaderChild final : public WinMultiprocess { } void WinMultiprocessChild() override { - WinVMAddress address = reinterpret_cast(kTestMemory); + WinVMAddress address = FromPointerCast(kTestMemory); CheckedWriteFile(WritePipeHandle(), &address, sizeof(address)); // Wait for the parent to signal that it's OK to exit by closing its end of diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index 11df2b80..f580fd09 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -26,6 +26,7 @@ #include "snapshot/win/exception_snapshot_win.h" #include "snapshot/win/memory_snapshot_win.h" #include "snapshot/win/module_snapshot_win.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/nt_internals.h" #include "util/win/registration_protocol_win.h" #include "util/win/time.h" @@ -295,7 +296,7 @@ void ProcessSnapshotWin::InitializeUnloadedModules() { } const WinVMAddress address_in_target_process = - reinterpret_cast(event_trace_address); + FromPointerCast(event_trace_address); Traits::Pointer pointer_to_array; if (!process_reader_.ReadMemory(address_in_target_process, diff --git a/util/mach/task_memory_test.cc b/util/mach/task_memory_test.cc index 4360d680..514f48f1 100644 --- a/util/mach/task_memory_test.cc +++ b/util/mach/task_memory_test.cc @@ -25,6 +25,7 @@ #include "base/mac/scoped_mach_vm.h" #include "gtest/gtest.h" #include "test/mac/mach_errors.h" +#include "util/misc/from_pointer_cast.h" namespace crashpad { namespace test { @@ -165,7 +166,7 @@ TEST(TaskMemory, ReadSelfUnmapped) { bool ReadCStringSelf(TaskMemory* memory, const char* pointer, std::string* result) { - return memory->ReadCString(reinterpret_cast(pointer), + return memory->ReadCString(FromPointerCast(pointer), result); } @@ -272,7 +273,7 @@ bool ReadCStringSizeLimitedSelf(TaskMemory* memory, size_t size, std::string* result) { return memory->ReadCStringSizeLimited( - reinterpret_cast(pointer), size, result); + FromPointerCast(pointer), size, result); } TEST(TaskMemory, ReadCStringSizeLimited_ConstCharEmpty) { @@ -462,7 +463,7 @@ TEST(TaskMemory, MappedMemoryDeallocates) { static const char kTestBuffer[] = "hello!"; mach_vm_address_t test_address = - reinterpret_cast(&kTestBuffer); + FromPointerCast(&kTestBuffer); ASSERT_TRUE((mapped = memory.ReadMapped(test_address, sizeof(kTestBuffer)))); EXPECT_EQ(memcmp(kTestBuffer, mapped->data(), sizeof(kTestBuffer)), 0); @@ -477,7 +478,7 @@ TEST(TaskMemory, MappedMemoryDeallocates) { // deallocated. const size_t kBigSize = 4 * PAGE_SIZE; std::unique_ptr big_buffer(new char[kBigSize]); - test_address = reinterpret_cast(&big_buffer[0]); + test_address = FromPointerCast(&big_buffer[0]); ASSERT_TRUE((mapped = memory.ReadMapped(test_address, kBigSize))); mapped_address = reinterpret_cast(mapped->data()); @@ -499,7 +500,7 @@ TEST(TaskMemory, MappedMemoryReadCString) { static const char kTestBuffer[] = "0\0" "2\0" "45\0" "789"; const mach_vm_address_t kTestAddress = - reinterpret_cast(&kTestBuffer); + FromPointerCast(&kTestBuffer); ASSERT_TRUE((mapped = memory.ReadMapped(kTestAddress, 10))); std::string string; diff --git a/util/misc/from_pointer_cast.h b/util/misc/from_pointer_cast.h index 3f42447d..f1b37349 100644 --- a/util/misc/from_pointer_cast.h +++ b/util/misc/from_pointer_cast.h @@ -20,6 +20,8 @@ #include #include +#include "base/numerics/safe_conversions.h" + namespace crashpad { #if DOXYGEN @@ -29,37 +31,29 @@ namespace crashpad { //! Compared to `reinterpret_cast<>()`, FromPointerCast<>() defines whether a //! pointer type is sign-extended or zero-extended. Casts to signed integral //! types are sign-extended. Casts to unsigned integral types are zero-extended. +//! +//! Use FromPointerCast<>() instead of `reinterpret_cast<>()` when casting a +//! pointer to an integral type that may not be the same width as a pointer. +//! There is no need to prefer FromPointerCast<>() when casting to an integral +//! type that’s definitely the same width as a pointer, such as `uintptr_t` and +//! `intptr_t`. template -FromPointerCast(const From from) { +FromPointerCast(From from) { return reinterpret_cast(from); } #else // DOXYGEN -// Cast std::nullptr_t to any pointer type. +// Cast std::nullptr_t to any type. // -// In C++14, the nullptr_t check could use std::is_null_pointer::type +// In C++14, the nullptr_t check could use std::is_null_pointer::value // instead of the is_same::type, nullptr_t>::type construct. template typename std::enable_if< - std::is_same::type, std::nullptr_t>::value && - std::is_pointer::value, + std::is_same::type, std::nullptr_t>::value, To>::type -FromPointerCast(const From& from) { - return static_cast(from); -} - -// Cast std::nullptr_t to any integral type. -// -// In C++14, the nullptr_t check could use std::is_null_pointer::type -// instead of the is_same::type, nullptr_t>::type construct. -template -typename std::enable_if< - std::is_same::type, std::nullptr_t>::value && - std::is_integral::value, - To>::type -FromPointerCast(const From& from) { - return reinterpret_cast(from); +FromPointerCast(From) { + return To(); } // Cast a pointer to any other pointer type. @@ -67,7 +61,7 @@ template typename std::enable_if::value && std::is_pointer::value, To>::type -FromPointerCast(const From from) { +FromPointerCast(From from) { return reinterpret_cast(from); } @@ -77,11 +71,21 @@ template typename std::enable_if::value && std::is_integral::value, To>::type -FromPointerCast(const From from) { - return static_cast( +FromPointerCast(From from) { + const auto intermediate = reinterpret_cast::value, intptr_t, - uintptr_t>::type>(from)); + uintptr_t>::type>(from); + + if (sizeof(To) >= sizeof(From)) { + // If the destination integral type is at least as wide as the source + // pointer type, use static_cast<>() and just return it. + return static_cast(intermediate); + } + + // If the destination integral type is narrower than the source pointer type, + // use checked_cast<>(). + return base::checked_cast(intermediate); } #endif // DOXYGEN diff --git a/util/misc/from_pointer_cast_test.cc b/util/misc/from_pointer_cast_test.cc index 0c90c275..b961abba 100644 --- a/util/misc/from_pointer_cast_test.cc +++ b/util/misc/from_pointer_cast_test.cc @@ -21,6 +21,7 @@ #include "build/build_config.h" #include "gtest/gtest.h" +#include "test/gtest_death_check.h" namespace crashpad { namespace test { @@ -185,6 +186,76 @@ TEST(FromPointerCast, FromFunctionPointer) { MaybeRemoveVolatile(reinterpret_cast(malloc))); } +TEST(FromPointerCast, ToNarrowInteger) { + EXPECT_EQ(FromPointerCast(nullptr), 0); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1); + EXPECT_EQ(FromPointerCast(reinterpret_cast(-1)), -1); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::min()))), + std::numeric_limits::min()); + + EXPECT_EQ(FromPointerCast(nullptr), 0u); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1u); + EXPECT_EQ( + FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + static_cast(std::numeric_limits::max())); + + // int and unsigned int may not be narrower than a pointer, so also test short + // and unsigned short. + + EXPECT_EQ(FromPointerCast(nullptr), 0); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1); + EXPECT_EQ(FromPointerCast(reinterpret_cast(-1)), -1); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::min()))), + std::numeric_limits::min()); + + EXPECT_EQ(FromPointerCast(nullptr), 0u); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1u); + EXPECT_EQ( + FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + static_cast(std::numeric_limits::max())); +} + +TEST(FromPointerCastDeathTest, ToNarrowInteger) { + if (sizeof(int) < sizeof(void*)) { + EXPECT_DEATH(FromPointerCast( + reinterpret_cast(static_cast( + std::numeric_limits::max() + 1ull))), + ""); + EXPECT_DEATH(FromPointerCast( + reinterpret_cast(static_cast( + std::numeric_limits::max() + 1ull))), + ""); + } + + // int and unsigned int may not be narrower than a pointer, so also test short + // and unsigned short. + + EXPECT_DEATH(FromPointerCast( + reinterpret_cast(static_cast( + std::numeric_limits::max() + 1u))), + ""); + EXPECT_DEATH(FromPointerCast( + reinterpret_cast(static_cast( + std::numeric_limits::max() + 1u))), + ""); +} + } // namespace } // namespace test } // namespace crashpad diff --git a/util/win/handle.cc b/util/win/handle.cc index c53f5438..1744ecc2 100644 --- a/util/win/handle.cc +++ b/util/win/handle.cc @@ -17,6 +17,7 @@ #include #include "base/numerics/safe_conversions.h" +#include "util/misc/from_pointer_cast.h" namespace crashpad { @@ -26,7 +27,7 @@ namespace crashpad { // back to the same HANDLE value. int HandleToInt(HANDLE handle) { - return base::checked_cast(reinterpret_cast(handle)); + return FromPointerCast(handle); } HANDLE IntToHandle(int handle_int) { diff --git a/util/win/process_info.cc b/util/win/process_info.cc index f0e455db..d3871c2d 100644 --- a/util/win/process_info.cc +++ b/util/win/process_info.cc @@ -28,6 +28,7 @@ #include "base/process/memory.h" #include "base/strings/stringprintf.h" #include "build/build_config.h" +#include "util/misc/from_pointer_cast.h" #include "util/numeric/safe_assignment.h" #include "util/win/get_function.h" #include "util/win/handle.h" @@ -133,7 +134,7 @@ bool RegionIsAccessible(const MEMORY_BASIC_INFORMATION64& memory_info) { MEMORY_BASIC_INFORMATION64 MemoryBasicInformationToMemoryBasicInformation64( const MEMORY_BASIC_INFORMATION& mbi) { MEMORY_BASIC_INFORMATION64 mbi64 = {0}; - mbi64.BaseAddress = reinterpret_cast(mbi.BaseAddress); + mbi64.BaseAddress = FromPointerCast(mbi.BaseAddress); mbi64.AllocationBase = reinterpret_cast(mbi.AllocationBase); mbi64.AllocationProtect = mbi.AllocationProtect; mbi64.RegionSize = mbi.RegionSize; diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index 322a987f..50f932c8 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -30,6 +30,7 @@ #include "test/test_paths.h" #include "test/win/child_launcher.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/random_string.h" #include "util/misc/uuid.h" #include "util/win/command_line.h" @@ -125,7 +126,7 @@ TEST(ProcessInfo, Self) { // Find something we know is a code address and confirm expected memory // information settings. VerifyAddressInInCodePage(process_info, - reinterpret_cast(_ReturnAddress())); + FromPointerCast(_ReturnAddress())); } void TestOtherProcess(const base::string16& directory_modification) { @@ -640,7 +641,7 @@ TEST(ProcessInfo, OutOfRangeCheck) { EXPECT_TRUE( info.LoggingRangeIsFullyReadable(CheckedRange( - reinterpret_cast(safe_memory.get()), kAllocationSize))); + FromPointerCast(safe_memory.get()), kAllocationSize))); EXPECT_FALSE(info.LoggingRangeIsFullyReadable( CheckedRange(0, 1024))); }