From b666bcbe98c67256a64418313755a40fea2992d6 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 6 Nov 2015 15:03:13 -0500 Subject: [PATCH] win: Use signed int as the integer representation of HANDLEs HandleToInt() and IntToHandle() use int, a signed type, for the 32-bit integer representation of HANDLE values. For opaque values, an unsigned type would normally be used, but in this case, signed was chosen for sign extension to work correctly. INVALID_HANDLE_VALUE is defined as ((HANDLE)(LONG_PTR)-1), and this needs to round-trip through the chosen integer representation back to the same HANDLE value. Sign extension is also recommended by https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203. As suggested in https://codereview.chromium.org/1422503015/diff/1/util/win/handle.cc#newcode24 R=scottmg@chromium.org Review URL: https://codereview.chromium.org/1422023010 . --- test/win/win_child_process.cc | 4 ++++ util/win/handle.h | 24 ++++++++++++------------ util/win/process_info.h | 2 +- util/win/process_info_test.cc | 9 ++++----- util/win/registration_protocol_win.h | 20 ++++++++++---------- 5 files changed, 31 insertions(+), 28 deletions(-) diff --git a/test/win/win_child_process.cc b/test/win/win_child_process.cc index b6cbbbd2..6b7a1a07 100644 --- a/test/win/win_child_process.cc +++ b/test/win/win_child_process.cc @@ -142,9 +142,13 @@ WinChildProcess::WinChildProcess() { // values are passed to the child on the command line. std::string left, right; CHECK(SplitString(switch_value, '|', &left, &right)); + + // left and right were formatted as 0x%x, so they need to be converted as + // unsigned ints. unsigned int write, read; CHECK(StringToNumber(left, &write)); CHECK(StringToNumber(right, &read)); + pipe_write_.reset(IntToHandle(write)); pipe_read_.reset(IntToHandle(read)); diff --git a/util/win/handle.h b/util/win/handle.h index c5a96cae..8a630690 100644 --- a/util/win/handle.h +++ b/util/win/handle.h @@ -21,17 +21,17 @@ namespace crashpad { //! \brief Converts a `HANDLE` to an `int`. //! -//! `HANDLE` is a `typedef` for `void *`, but `HANDLE` values aren’t necessarily -//! pointers to anything. Only 32 bits of shareable `HANDLE`s are significant, -//! even in 64-bit processes on 64-bit operating systems. See Interprocess //! Communication Between 32-bit and 64-bit Applications. //! -//! This function safely converts a shareable `HANDLE` to an `int` similarly to -//! a cast operation. It checks that the operation can be performed safely, and +//! This function safely converts a kernel `HANDLE` to an `int` similarly to a +//! cast operation. It checks that the operation can be performed safely, and //! aborts execution if it cannot. //! -//! \param[in] handle The shareable `HANDLE` to convert. +//! \param[in] handle The kernel `HANDLE` to convert. //! //! \return An equivalent `int`, truncated (if necessary) from \a handle. If //! truncation would have resulted in an `int` that could not be converted @@ -42,20 +42,20 @@ int HandleToInt(HANDLE handle); //! \brief Converts an `int` to an `HANDLE`. //! -//! `HANDLE` is a `typedef` for `void *`, but `HANDLE` values aren’t necessarily -//! pointers to anything. Only 32 bits of shareable `HANDLE`s are significant, -//! even in 64-bit processes on 64-bit operating systems. See Interprocess //! Communication Between 32-bit and 64-bit Applications. //! -//! This function safely convert an `int` to a shareable `HANDLE` similarly to a +//! This function safely convert an `int` to a kernel `HANDLE` similarly to a //! cast operation. //! //! \param[in] handle_int The `int` to convert. This must have been produced by //! HandleToInt(), possibly in a different process. //! -//! \return An equivalent shareable `HANDLE`, sign-extended (if necessary) from -//! \a handle_int. +//! \return An equivalent kernel `HANDLE`, sign-extended (if necessary) from \a +//! handle_int. //! //! \sa HandleToInt() HANDLE IntToHandle(int handle_int); diff --git a/util/win/process_info.h b/util/win/process_info.h index 49a6f354..fb1b8b3e 100644 --- a/util/win/process_info.h +++ b/util/win/process_info.h @@ -58,7 +58,7 @@ class ProcessInfo { std::wstring type_name; //! \brief The handle's value. - uint32_t handle; + int handle; //! \brief The attributes for the handle, e.g. `OBJ_INHERIT`, //! `OBJ_CASE_INSENSITIVE`, etc. diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index 7bf0f4ef..74f086db 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -576,8 +576,7 @@ TEST(ProcessInfo, Handles) { bool found_key_handle = false; bool found_mapping_handle = false; for (auto handle : info.Handles()) { - const int handle_int = implicit_cast(handle.handle); - if (handle_int == HandleToInt(file.get())) { + if (handle.handle == HandleToInt(file.get())) { EXPECT_FALSE(found_file_handle); found_file_handle = true; EXPECT_EQ(L"File", handle.type_name); @@ -587,7 +586,7 @@ TEST(ProcessInfo, Handles) { handle.granted_access & STANDARD_RIGHTS_ALL); EXPECT_EQ(0, handle.attributes); } - if (handle_int == HandleToInt(inherited_file.get())) { + if (handle.handle == HandleToInt(inherited_file.get())) { EXPECT_FALSE(found_inherited_file_handle); found_inherited_file_handle = true; EXPECT_EQ(L"File", handle.type_name); @@ -601,7 +600,7 @@ TEST(ProcessInfo, Handles) { const int kObjInherit = 0x2; EXPECT_EQ(kObjInherit, handle.attributes); } - if (handle_int == HandleToInt(scoped_key.get())) { + if (handle.handle == HandleToInt(scoped_key.get())) { EXPECT_FALSE(found_key_handle); found_key_handle = true; EXPECT_EQ(L"Key", handle.type_name); @@ -611,7 +610,7 @@ TEST(ProcessInfo, Handles) { handle.granted_access & STANDARD_RIGHTS_ALL); EXPECT_EQ(0, handle.attributes); } - if (handle_int == HandleToInt(mapping.get())) { + if (handle.handle == HandleToInt(mapping.get())) { EXPECT_FALSE(found_mapping_handle); found_mapping_handle = true; EXPECT_EQ(L"Section", handle.type_name); diff --git a/util/win/registration_protocol_win.h b/util/win/registration_protocol_win.h index f2d9e29c..c57c6e20 100644 --- a/util/win/registration_protocol_win.h +++ b/util/win/registration_protocol_win.h @@ -92,22 +92,22 @@ struct ClientToServerMessage { }; }; -//! \brief A client registration response. +//! \brief A client registration response. struct RegistrationResponse { //! \brief An event `HANDLE`, valid in the client process, that should be - //! signaled to request a crash report. 64-bit clients should convert the - //! value to a `HANDLE` using sign-extension. - uint32_t request_crash_dump_event; + //! signaled to request a crash report. Clients should convert the value + //! to a `HANDLE` by calling IntToHandle(). + int request_crash_dump_event; //! \brief An event `HANDLE`, valid in the client process, that should be - //! signaled to request a non-crashing dump be taken. 64-bit clients - //! should convert the value to `HANDLEEE` using sign-extension. - uint32_t request_non_crash_dump_event; + //! signaled to request a non-crashing dump be taken. Clients should + //! convert the value to a `HANDLE` by calling IntToHandle(). + int request_non_crash_dump_event; //! \brief An event `HANDLE`, valid in the client process, that will be - //! signaled by the server when the non-crashing dump is complete. 64-bit - //! clients should convert the value to `HANDLEEE` using sign-extension. - uint32_t non_crash_dump_completed_event; + //! signaled by the server when the non-crashing dump is complete. Clients + //! should convert the value to a `HANDLE` by calling IntToHandle(). + int non_crash_dump_completed_event; }; //! \brief The response sent back to the client via SendToCrashHandlerServer().