From 61542fa828eea7bc31bb8d3eeefbf00acf0fc43a Mon Sep 17 00:00:00 2001 From: "pkasting@chromium.org" Date: Thu, 10 Jul 2014 22:56:22 +0000 Subject: [PATCH] Fixes for re-enabling more MSVC level 4 warnings: sandbox/ edition This contains fixes for the following sorts of issues: * Assignment inside conditional * Possibly-uninitialized local variable * Signedness mismatch This also contains a small number of other cleanups/simplifications to nearby code. BUG=81439 TEST=none Review URL: https://codereview.chromium.org/382613002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282451 0039d316-1c4b-4281-b951-d872f2087c98 --- sandbox/win/src/handle_closer_test.cc | 3 ++- sandbox/win/src/handle_dispatcher.cc | 6 +++--- sandbox/win/src/handle_interception.cc | 2 +- sandbox/win/src/process_policy_test.cc | 24 ++++++++---------------- sandbox/win/src/sync_dispatcher.cc | 18 +++++++----------- sandbox/win/src/sync_policy.cc | 22 +++++++++++----------- sandbox/win/src/sync_policy.h | 22 +++++++++++----------- sandbox/win/tests/common/controller.cc | 4 ++-- sandbox/win/tests/validation_tests/commands.cc | 4 ++-- 9 files changed, 47 insertions(+), 58 deletions(-) diff --git a/sandbox/win/src/handle_closer_test.cc b/sandbox/win/src/handle_closer_test.cc index 9adcf6c62143..8e821ced73fe 100644 --- a/sandbox/win/src/handle_closer_test.cc +++ b/sandbox/win/src/handle_closer_test.cc @@ -157,7 +157,8 @@ void WINAPI ThreadPoolTask(void* event, BOOLEAN timeout) { // Run a thread pool inside a sandbox without a CSRSS connection. SBOX_TESTS_COMMAND int RunThreadPool(int argc, wchar_t **argv) { HANDLE wait_list[20]; - CHECK(finish_event = ::CreateEvent(NULL, TRUE, FALSE, NULL)); + finish_event = ::CreateEvent(NULL, TRUE, FALSE, NULL); + CHECK(finish_event); // Set up a bunch of waiters. HANDLE pool = NULL; diff --git a/sandbox/win/src/handle_dispatcher.cc b/sandbox/win/src/handle_dispatcher.cc index 6acb6f9ceb30..fda7aacaaef3 100644 --- a/sandbox/win/src/handle_dispatcher.cc +++ b/sandbox/win/src/handle_dispatcher.cc @@ -44,7 +44,6 @@ bool HandleDispatcher::DuplicateHandleProxy(IPCInfo* ipc, DWORD target_process_id, DWORD desired_access, DWORD options) { - NTSTATUS error; static NtQueryObject QueryObject = NULL; if (!QueryObject) ResolveNTFunctionPtr("NtQueryObject", &QueryObject); @@ -65,9 +64,10 @@ bool HandleDispatcher::DuplicateHandleProxy(IPCInfo* ipc, OBJECT_TYPE_INFORMATION* type_info = reinterpret_cast(buffer); ULONG size = sizeof(buffer) - sizeof(wchar_t); - error = QueryObject(handle, ObjectTypeInformation, type_info, size, &size); + NTSTATUS error = + QueryObject(handle, ObjectTypeInformation, type_info, size, &size); if (!NT_SUCCESS(error)) { - ipc->return_info.win32_result = error; + ipc->return_info.nt_status = error; return false; } type_info->Name.Buffer[type_info->Name.Length / sizeof(wchar_t)] = L'\0'; diff --git a/sandbox/win/src/handle_interception.cc b/sandbox/win/src/handle_interception.cc index af0bebca0451..a0df8d653d40 100644 --- a/sandbox/win/src/handle_interception.cc +++ b/sandbox/win/src/handle_interception.cc @@ -33,7 +33,7 @@ ResultCode DuplicateHandleProxy(HANDLE source_handle, return code; if (answer.win32_result) { - ::SetLastError(answer.nt_status); + ::SetLastError(answer.win32_result); return SBOX_ERROR_GENERIC; } diff --git a/sandbox/win/src/process_policy_test.cc b/sandbox/win/src/process_policy_test.cc index af64f14f7912..ae6260633aca 100644 --- a/sandbox/win/src/process_policy_test.cc +++ b/sandbox/win/src/process_policy_test.cc @@ -154,28 +154,20 @@ SBOX_TESTS_COMMAND int Process_RunApp4(int argc, wchar_t **argv) { // TEST 4: Try file name in the app_name and current directory sets correctly. base::string16 system32 = MakeFullPathToSystem32(L""); wchar_t current_directory[MAX_PATH + 1]; - int result4; - bool test_succeeded = false; DWORD ret = ::GetCurrentDirectory(MAX_PATH, current_directory); if (!ret) return SBOX_TEST_FIRST_ERROR; + if (ret >= MAX_PATH) + return SBOX_TEST_FAILED; - if (ret < MAX_PATH) { - current_directory[ret] = L'\\'; - current_directory[ret+1] = L'\0'; - if (::SetCurrentDirectory(system32.c_str())) { - result4 = CreateProcessHelper(argv[0], base::string16()); - if (::SetCurrentDirectory(current_directory)) { - test_succeeded = true; - } - } else { - return SBOX_TEST_SECOND_ERROR; - } + current_directory[ret] = L'\\'; + current_directory[ret+1] = L'\0'; + if (!::SetCurrentDirectory(system32.c_str())) { + return SBOX_TEST_SECOND_ERROR; } - if (!test_succeeded) - result4 = SBOX_TEST_FAILED; - return result4; + const int result4 = CreateProcessHelper(argv[0], base::string16()); + return ::SetCurrentDirectory(current_directory) ? result4 : SBOX_TEST_FAILED; } SBOX_TESTS_COMMAND int Process_RunApp5(int argc, wchar_t **argv) { diff --git a/sandbox/win/src/sync_dispatcher.cc b/sandbox/win/src/sync_dispatcher.cc index d4b36d5fc850..18a07439df91 100644 --- a/sandbox/win/src/sync_dispatcher.cc +++ b/sandbox/win/src/sync_dispatcher.cc @@ -35,12 +35,11 @@ SyncDispatcher::SyncDispatcher(PolicyBase* policy_base) bool SyncDispatcher::SetupService(InterceptionManager* manager, int service) { - if (IPC_CREATEEVENT_TAG == service) { + if (service == IPC_CREATEEVENT_TAG) { return INTERCEPT_NT(manager, NtCreateEvent, CREATE_EVENT_ID, 24); - } else if (IPC_OPENEVENT_TAG == service) { - return INTERCEPT_NT(manager, NtOpenEvent, OPEN_EVENT_ID, 16); } - return false; + return (service == IPC_OPENEVENT_TAG) && + INTERCEPT_NT(manager, NtOpenEvent, OPEN_EVENT_ID, 16); } bool SyncDispatcher::CreateEvent(IPCInfo* ipc, base::string16* name, @@ -52,11 +51,9 @@ bool SyncDispatcher::CreateEvent(IPCInfo* ipc, base::string16* name, EvalResult result = policy_base_->EvalPolicy(IPC_CREATEEVENT_TAG, params.GetBase()); HANDLE handle = NULL; - DWORD ret = SyncPolicy::CreateEventAction(result, *ipc->client_info, *name, - event_type, initial_state, - &handle); // Return operation status on the IPC. - ipc->return_info.nt_status = ret; + ipc->return_info.nt_status = SyncPolicy::CreateEventAction( + result, *ipc->client_info, *name, event_type, initial_state, &handle); ipc->return_info.handle = handle; return true; } @@ -72,10 +69,9 @@ bool SyncDispatcher::OpenEvent(IPCInfo* ipc, base::string16* name, EvalResult result = policy_base_->EvalPolicy(IPC_OPENEVENT_TAG, params.GetBase()); HANDLE handle = NULL; - DWORD ret = SyncPolicy::OpenEventAction(result, *ipc->client_info, *name, - desired_access, &handle); // Return operation status on the IPC. - ipc->return_info.win32_result = ret; + ipc->return_info.nt_status = SyncPolicy::OpenEventAction( + result, *ipc->client_info, *name, desired_access, &handle); ipc->return_info.handle = handle; return true; } diff --git a/sandbox/win/src/sync_policy.cc b/sandbox/win/src/sync_policy.cc index 7b18fe7078e5..75399e6f3ed5 100644 --- a/sandbox/win/src/sync_policy.cc +++ b/sandbox/win/src/sync_policy.cc @@ -176,12 +176,12 @@ bool SyncPolicy::GenerateRules(const wchar_t* name, return true; } -DWORD SyncPolicy::CreateEventAction(EvalResult eval_result, - const ClientInfo& client_info, - const base::string16 &event_name, - uint32 event_type, - uint32 initial_state, - HANDLE *handle) { +NTSTATUS SyncPolicy::CreateEventAction(EvalResult eval_result, + const ClientInfo& client_info, + const base::string16 &event_name, + uint32 event_type, + uint32 initial_state, + HANDLE *handle) { NtCreateEventFunction NtCreateEvent = NULL; ResolveNTFunctionPtr("NtCreateEvent", &NtCreateEvent); @@ -214,11 +214,11 @@ DWORD SyncPolicy::CreateEventAction(EvalResult eval_result, return status; } -DWORD SyncPolicy::OpenEventAction(EvalResult eval_result, - const ClientInfo& client_info, - const base::string16 &event_name, - uint32 desired_access, - HANDLE *handle) { +NTSTATUS SyncPolicy::OpenEventAction(EvalResult eval_result, + const ClientInfo& client_info, + const base::string16 &event_name, + uint32 desired_access, + HANDLE *handle) { NtOpenEventFunction NtOpenEvent = NULL; ResolveNTFunctionPtr("NtOpenEvent", &NtOpenEvent); diff --git a/sandbox/win/src/sync_policy.h b/sandbox/win/src/sync_policy.h index 4383998205f9..e370e4bdacb0 100644 --- a/sandbox/win/src/sync_policy.h +++ b/sandbox/win/src/sync_policy.h @@ -33,17 +33,17 @@ class SyncPolicy { // Performs the desired policy action on a request. // client_info is the target process that is making the request and // eval_result is the desired policy action to accomplish. - static DWORD CreateEventAction(EvalResult eval_result, - const ClientInfo& client_info, - const base::string16 &event_name, - uint32 event_type, - uint32 initial_state, - HANDLE *handle); - static DWORD OpenEventAction(EvalResult eval_result, - const ClientInfo& client_info, - const base::string16 &event_name, - uint32 desired_access, - HANDLE *handle); + static NTSTATUS CreateEventAction(EvalResult eval_result, + const ClientInfo& client_info, + const base::string16 &event_name, + uint32 event_type, + uint32 initial_state, + HANDLE *handle); + static NTSTATUS OpenEventAction(EvalResult eval_result, + const ClientInfo& client_info, + const base::string16 &event_name, + uint32 desired_access, + HANDLE *handle); }; } // namespace sandbox diff --git a/sandbox/win/tests/common/controller.cc b/sandbox/win/tests/common/controller.cc index ac20f6c7b5b2..8d355b6549bb 100644 --- a/sandbox/win/tests/common/controller.cc +++ b/sandbox/win/tests/common/controller.cc @@ -247,13 +247,13 @@ int TestRunner::InternalRunTest(const wchar_t* command) { } if (WAIT_TIMEOUT == ::WaitForSingleObject(target.hProcess, timeout_)) { - ::TerminateProcess(target.hProcess, SBOX_TEST_TIMED_OUT); + ::TerminateProcess(target.hProcess, static_cast(SBOX_TEST_TIMED_OUT)); ::CloseHandle(target.hProcess); ::CloseHandle(target.hThread); return SBOX_TEST_TIMED_OUT; } - DWORD exit_code = SBOX_TEST_LAST_RESULT; + DWORD exit_code = static_cast(SBOX_TEST_LAST_RESULT); if (!::GetExitCodeProcess(target.hProcess, &exit_code)) { ::CloseHandle(target.hProcess); ::CloseHandle(target.hThread); diff --git a/sandbox/win/tests/validation_tests/commands.cc b/sandbox/win/tests/validation_tests/commands.cc index 26133477267b..45717b472db2 100644 --- a/sandbox/win/tests/validation_tests/commands.cc +++ b/sandbox/win/tests/validation_tests/commands.cc @@ -276,8 +276,8 @@ int TestOpenAlternateDesktop(wchar_t *desktop_name) { } // Open by name with WRITE_DAC. - if ((desktop = ::OpenDesktop(desktop_name, 0, FALSE, WRITE_DAC)) || - ::GetLastError() != ERROR_ACCESS_DENIED) { + desktop = ::OpenDesktop(desktop_name, 0, FALSE, WRITE_DAC); + if (desktop || ::GetLastError() != ERROR_ACCESS_DENIED) { ::CloseDesktop(desktop); return SBOX_TEST_SUCCEEDED; } -- 2.11.4.GIT