diff options
author | pkasting <pkasting@chromium.org> | 2014-10-02 15:38:36 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-10-02 22:38:54 +0000 |
commit | 84995b8be698afc758352954fccd0bec5d971b65 (patch) | |
tree | 39431795d3a7e8a3e0523ec82c0d68976d71f812 | |
parent | c9bb939b8326bf72cb241b3eefa5d428906a043c (diff) | |
download | chromium_src-84995b8be698afc758352954fccd0bec5d971b65.zip chromium_src-84995b8be698afc758352954fccd0bec5d971b65.tar.gz chromium_src-84995b8be698afc758352954fccd0bec5d971b65.tar.bz2 |
Cleanup sandbox/ in advance of re-enabling some MSVC warnings.
In most files this means removing unused variables/code.
BUG=none
TEST=none
Review URL: https://codereview.chromium.org/614813002
Cr-Commit-Position: refs/heads/master@{#297931}
-rw-r--r-- | sandbox/win/src/eat_resolver.cc | 3 | ||||
-rw-r--r-- | sandbox/win/src/handle_closer_test.cc | 1 | ||||
-rw-r--r-- | sandbox/win/src/handle_policy.cc | 1 | ||||
-rw-r--r-- | sandbox/win/src/policy_opcodes_unittest.cc | 6 | ||||
-rw-r--r-- | sandbox/win/src/process_mitigations.cc | 8 | ||||
-rw-r--r-- | sandbox/win/src/process_mitigations_test.cc | 8 | ||||
-rw-r--r-- | sandbox/win/src/service_resolver_64.cc | 5 | ||||
-rw-r--r-- | sandbox/win/src/target_process.cc | 10 | ||||
-rw-r--r-- | sandbox/win/tests/validation_tests/commands.cc | 138 | ||||
-rw-r--r-- | sandbox/win/tests/validation_tests/suite.cc | 3 |
10 files changed, 67 insertions, 116 deletions
diff --git a/sandbox/win/src/eat_resolver.cc b/sandbox/win/src/eat_resolver.cc index 154bfcf..328ee00 100644 --- a/sandbox/win/src/eat_resolver.cc +++ b/sandbox/win/src/eat_resolver.cc @@ -26,13 +26,12 @@ NTSTATUS EatResolverThunk::Setup(const void* target_module, if (!eat_entry_) return STATUS_INVALID_PARAMETER; - size_t thunk_bytes = GetInternalThunkSize(); - #if defined(_WIN64) // We have two thunks, in order: the return path and the forward path. if (!SetInternalThunk(thunk_storage, storage_bytes, NULL, target_)) return STATUS_BUFFER_TOO_SMALL; + size_t thunk_bytes = GetInternalThunkSize(); storage_bytes -= thunk_bytes; thunk_storage = reinterpret_cast<char*>(thunk_storage) + thunk_bytes; #endif diff --git a/sandbox/win/src/handle_closer_test.cc b/sandbox/win/src/handle_closer_test.cc index 2f5890d..5b7be47 100644 --- a/sandbox/win/src/handle_closer_test.cc +++ b/sandbox/win/src/handle_closer_test.cc @@ -108,7 +108,6 @@ TEST(HandleCloserTest, CheckForMarkerFiles) { TestRunner runner; runner.SetTimeout(2000); runner.SetTestState(EVERY_STATE); - sandbox::TargetPolicy* policy = runner.GetPolicy(); base::string16 command = base::string16(L"CheckForFileHandles Y"); for (int i = 0; i < arraysize(kFileExtensions); ++i) { diff --git a/sandbox/win/src/handle_policy.cc b/sandbox/win/src/handle_policy.cc index f5f1c27..1023030 100644 --- a/sandbox/win/src/handle_policy.cc +++ b/sandbox/win/src/handle_policy.cc @@ -79,7 +79,6 @@ DWORD HandlePolicy::DuplicateHandleProxyAction(EvalResult eval_result, // (this process) is the valid target. HANDLE target_process = remote_target_process.IsValid() ? remote_target_process.Get() : ::GetCurrentProcess(); - DWORD result = ERROR_SUCCESS; if (!::DuplicateHandle(::GetCurrentProcess(), source_handle, target_process, target_handle, desired_access, FALSE, options)) { diff --git a/sandbox/win/src/policy_opcodes_unittest.cc b/sandbox/win/src/policy_opcodes_unittest.cc index c69aad8..16fe064 100644 --- a/sandbox/win/src/policy_opcodes_unittest.cc +++ b/sandbox/win/src/policy_opcodes_unittest.cc @@ -286,22 +286,26 @@ TEST(PolicyEngineTest, WCharOpcodes1) { CASE_SENSITIVE, kPolNone); EXPECT_EQ(EVAL_FALSE, op5b->Evaluate(&pp_tc1, 1, &mc1)); + EXPECT_EQ(24, mc1.position); // Test that we function if the string does not fit. In this case we // try to match 'the lazy dog' against 'he lazy dog'. + // !!! Are we supposed to Evaluate() this opcode and test the result? PolicyOpcode* op6 = opcode_maker.MakeOpWStringMatch(0, txt4, 2, CASE_SENSITIVE, kPolNone); - EXPECT_EQ(24, mc1.position); + EXPECT_EQ(EVAL_FALSE, op6->Evaluate(&pp_tc1, 1, &mc1)); // Testing matching against 'g' which should be the last char. MatchContext mc2; PolicyOpcode* op7 = opcode_maker.MakeOpWStringMatch(0, txt6, kSeekForward, CASE_SENSITIVE, kPolNone); EXPECT_EQ(EVAL_TRUE, op7->Evaluate(&pp_tc1, 1, &mc2)); + EXPECT_EQ(37, mc2.position); // Trying to match again should fail since we are in the last char. // This also covers a couple of boundary conditions. EXPECT_EQ(EVAL_FALSE, op7->Evaluate(&pp_tc1, 1, &mc2)); + EXPECT_EQ(37, mc2.position); } TEST(PolicyEngineTest, WCharOpcodes2) { diff --git a/sandbox/win/src/process_mitigations.cc b/sandbox/win/src/process_mitigations.cc index 5e242f3a..80e4284 100644 --- a/sandbox/win/src/process_mitigations.cc +++ b/sandbox/win/src/process_mitigations.cc @@ -79,10 +79,10 @@ bool ApplyProcessMitigationsToCurrentProcess(MitigationFlags flags) { } else { // We're on XP sp2, so use the less standard approach. // For reference: http://www.uninformed.org/?v=2&a=4 - const int MEM_EXECUTE_OPTION_ENABLE = 1; - const int MEM_EXECUTE_OPTION_DISABLE = 2; - const int MEM_EXECUTE_OPTION_ATL7_THUNK_EMULATION = 4; - const int MEM_EXECUTE_OPTION_PERMANENT = 8; + static const int MEM_EXECUTE_OPTION_ENABLE = 1; + static const int MEM_EXECUTE_OPTION_DISABLE = 2; + static const int MEM_EXECUTE_OPTION_ATL7_THUNK_EMULATION = 4; + static const int MEM_EXECUTE_OPTION_PERMANENT = 8; NtSetInformationProcessFunction set_information_process = NULL; ResolveNTFunctionPtr("NtSetInformationProcess", diff --git a/sandbox/win/src/process_mitigations_test.cc b/sandbox/win/src/process_mitigations_test.cc index e606256..4d2e9c6 100644 --- a/sandbox/win/src/process_mitigations_test.cc +++ b/sandbox/win/src/process_mitigations_test.cc @@ -165,10 +165,10 @@ SBOX_TESTS_COMMAND int CheckDep(int argc, wchar_t **argv) { return SBOX_TEST_THIRD_ERROR; } - const int MEM_EXECUTE_OPTION_ENABLE = 1; - const int MEM_EXECUTE_OPTION_DISABLE = 2; - const int MEM_EXECUTE_OPTION_ATL7_THUNK_EMULATION = 4; - const int MEM_EXECUTE_OPTION_PERMANENT = 8; + static const int MEM_EXECUTE_OPTION_ENABLE = 1; + static const int MEM_EXECUTE_OPTION_DISABLE = 2; + static const int MEM_EXECUTE_OPTION_ATL7_THUNK_EMULATION = 4; + static const int MEM_EXECUTE_OPTION_PERMANENT = 8; dep_flags &= 0xff; if (dep_flags != (MEM_EXECUTE_OPTION_DISABLE | diff --git a/sandbox/win/src/service_resolver_64.cc b/sandbox/win/src/service_resolver_64.cc index 03795f7..984cb38 100644 --- a/sandbox/win/src/service_resolver_64.cc +++ b/sandbox/win/src/service_resolver_64.cc @@ -168,11 +168,6 @@ bool ServiceResolverThunk::IsFunctionAService(void* local_thunk) const { NTSTATUS ServiceResolverThunk::PerformPatch(void* local_thunk, void* remote_thunk) { - ServiceFullThunk* full_local_thunk = - reinterpret_cast<ServiceFullThunk*>(local_thunk); - ServiceFullThunk* full_remote_thunk = - reinterpret_cast<ServiceFullThunk*>(remote_thunk); - // Patch the original code. ServiceEntry local_service; DCHECK_NT(GetInternalThunkSize() >= sizeof(local_service)); diff --git a/sandbox/win/src/target_process.cc b/sandbox/win/src/target_process.cc index fea52b2..2c5bf3b 100644 --- a/sandbox/win/src/target_process.cc +++ b/sandbox/win/src/target_process.cc @@ -88,14 +88,16 @@ TargetProcess::~TargetProcess() { // from showing up in purify. if (sandbox_process_info_.IsValid()) { ::WaitForSingleObject(sandbox_process_info_.process_handle(), 50); + // At this point, the target process should have been killed. Check. if (!::GetExitCodeProcess(sandbox_process_info_.process_handle(), &exit_code) || (STILL_ACTIVE == exit_code)) { - // It is an error to destroy this object while the target process is still - // alive because we need to destroy the IPC subsystem and cannot risk to - // have an IPC reach us after this point. + // Something went wrong. We don't know if the target is in a state where + // it can manage to do another IPC call. If it can, and we've destroyed + // the |ipc_server_|, it will crash the broker. So we intentionally leak + // that. if (shared_section_.IsValid()) shared_section_.Take(); - SharedMemIPCServer* server = ipc_server_.release(); + ipc_server_.release(); sandbox_process_info_.TakeProcessHandle(); return; } diff --git a/sandbox/win/tests/validation_tests/commands.cc b/sandbox/win/tests/validation_tests/commands.cc index f64465a..10a4a13 100644 --- a/sandbox/win/tests/validation_tests/commands.cc +++ b/sandbox/win/tests/validation_tests/commands.cc @@ -15,15 +15,15 @@ namespace { // Returns the HKEY corresponding to name. If there is no HKEY corresponding // to the name it returns NULL. HKEY GetHKEYFromString(const base::string16 &name) { - if (L"HKLM" == name) + if (name == L"HKLM") return HKEY_LOCAL_MACHINE; - else if (L"HKCR" == name) + if (name == L"HKCR") return HKEY_CLASSES_ROOT; - else if (L"HKCC" == name) + if (name == L"HKCC") return HKEY_CURRENT_CONFIG; - else if (L"HKCU" == name) + if (name == L"HKCU") return HKEY_CURRENT_USER; - else if (L"HKU" == name) + if (name == L"HKU") return HKEY_USERS; return NULL; @@ -34,8 +34,8 @@ void trim_quote(base::string16* string) { base::string16::size_type pos1 = string->find_first_not_of(L'"'); base::string16::size_type pos2 = string->find_last_not_of(L'"'); - if (base::string16::npos == pos1 || base::string16::npos == pos2) - (*string) = L""; + if (pos1 == base::string16::npos || pos2 == base::string16::npos) + string->clear(); else (*string) = string->substr(pos1, pos2 + 1); } @@ -56,16 +56,12 @@ int TestOpenFile(base::string16 path, bool for_write) { FILE_FLAG_BACKUP_SEMANTICS, NULL); // No template. - if (INVALID_HANDLE_VALUE != file) { + if (file != INVALID_HANDLE_VALUE) { ::CloseHandle(file); return sandbox::SBOX_TEST_SUCCEEDED; - } else { - if (ERROR_ACCESS_DENIED == ::GetLastError()) { - return sandbox::SBOX_TEST_DENIED; - } else { - return sandbox::SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; - } } + return (::GetLastError() == ERROR_ACCESS_DENIED) ? + sandbox::SBOX_TEST_DENIED : sandbox::SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; } } // namespace @@ -73,70 +69,49 @@ int TestOpenFile(base::string16 path, bool for_write) { namespace sandbox { SBOX_TESTS_COMMAND int ValidWindow(int argc, wchar_t **argv) { - if (1 != argc) - return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; - - HWND window = reinterpret_cast<HWND>(static_cast<ULONG_PTR>(_wtoi(argv[0]))); - - return TestValidWindow(window); + return (argc == 1) ? + TestValidWindow( + reinterpret_cast<HWND>(static_cast<ULONG_PTR>(_wtoi(argv[0])))) : + SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; } int TestValidWindow(HWND window) { - if (::IsWindow(window)) - return SBOX_TEST_SUCCEEDED; - - return SBOX_TEST_DENIED; + return ::IsWindow(window) ? SBOX_TEST_SUCCEEDED : SBOX_TEST_DENIED; } SBOX_TESTS_COMMAND int OpenProcessCmd(int argc, wchar_t **argv) { - if (2 != argc) - return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; - - DWORD process_id = _wtol(argv[0]); - DWORD access_mask = _wtol(argv[1]); - return TestOpenProcess(process_id, access_mask); + return (argc == 2) ? + TestOpenProcess(_wtol(argv[0]), _wtol(argv[1])) : + SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; } int TestOpenProcess(DWORD process_id, DWORD access_mask) { HANDLE process = ::OpenProcess(access_mask, FALSE, // Do not inherit handle. process_id); - if (NULL == process) { - if (ERROR_ACCESS_DENIED == ::GetLastError()) { - return SBOX_TEST_DENIED; - } else { - return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; - } - } else { + if (process != NULL) { ::CloseHandle(process); return SBOX_TEST_SUCCEEDED; } + return (::GetLastError() == ERROR_ACCESS_DENIED) ? + sandbox::SBOX_TEST_DENIED : sandbox::SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; } SBOX_TESTS_COMMAND int OpenThreadCmd(int argc, wchar_t **argv) { - if (1 != argc) - return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; - - DWORD thread_id = _wtoi(argv[0]); - return TestOpenThread(thread_id); + return (argc == 1) ? + TestOpenThread(_wtoi(argv[0])) : SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; } int TestOpenThread(DWORD thread_id) { - HANDLE thread = ::OpenThread(THREAD_QUERY_INFORMATION, FALSE, // Do not inherit handles. thread_id); - - if (NULL == thread) { - if (ERROR_ACCESS_DENIED == ::GetLastError()) { - return SBOX_TEST_DENIED; - } else { - return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; - } - } else { + if (thread != NULL) { ::CloseHandle(thread); return SBOX_TEST_SUCCEEDED; } + return (::GetLastError() == ERROR_ACCESS_DENIED) ? + sandbox::SBOX_TEST_DENIED : sandbox::SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; } SBOX_TESTS_COMMAND int OpenFileCmd(int argc, wchar_t **argv) { @@ -154,12 +129,11 @@ int TestOpenReadFile(const base::string16& path) { } int TestOpenWriteFile(int argc, wchar_t **argv) { - if (1 != argc) + if (argc != 1) return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; base::string16 path = argv[0]; trim_quote(&path); - return TestOpenWriteFile(path); } @@ -168,7 +142,7 @@ int TestOpenWriteFile(const base::string16& path) { } SBOX_TESTS_COMMAND int OpenKey(int argc, wchar_t **argv) { - if (0 == argc || argc > 2) + if (argc != 1 && argc != 2) return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; // Get the hive. @@ -176,7 +150,7 @@ SBOX_TESTS_COMMAND int OpenKey(int argc, wchar_t **argv) { // Get the subkey. base::string16 subkey; - if (2 == argc) { + if (argc == 2) { subkey = argv[1]; trim_quote(&subkey); } @@ -191,15 +165,12 @@ int TestOpenKey(HKEY base_key, base::string16 subkey) { 0, // Reserved, must be 0. MAXIMUM_ALLOWED, &key); - if (ERROR_SUCCESS == err_code) { + if (err_code == ERROR_SUCCESS) { ::RegCloseKey(key); return SBOX_TEST_SUCCEEDED; - } else if (ERROR_INVALID_HANDLE == err_code || - ERROR_ACCESS_DENIED == err_code) { - return SBOX_TEST_DENIED; - } else { - return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; } + return (err_code == ERROR_INVALID_HANDLE || err_code == ERROR_ACCESS_DENIED) ? + SBOX_TEST_DENIED : SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; } // Returns true if the current's thread desktop is the interactive desktop. @@ -207,14 +178,12 @@ int TestOpenKey(HKEY base_key, base::string16 subkey) { // the object name. bool IsInteractiveDesktop(bool* is_interactive) { HDESK current_desk = ::GetThreadDesktop(::GetCurrentThreadId()); - if (NULL == current_desk) { + if (current_desk == NULL) return false; - } wchar_t current_desk_name[256] = {0}; if (!::GetUserObjectInformationW(current_desk, UOI_NAME, current_desk_name, - sizeof(current_desk_name), NULL)) { + sizeof(current_desk_name), NULL)) return false; - } *is_interactive = (0 == _wcsicmp(L"default", current_desk_name)); return true; } @@ -225,9 +194,8 @@ SBOX_TESTS_COMMAND int OpenInteractiveDesktop(int, wchar_t **) { int TestOpenInputDesktop() { bool is_interactive = false; - if (IsInteractiveDesktop(&is_interactive) && is_interactive) { + if (IsInteractiveDesktop(&is_interactive) && is_interactive) return SBOX_TEST_SUCCEEDED; - } HDESK desk = ::OpenInputDesktop(0, FALSE, DESKTOP_CREATEWINDOW); if (desk) { ::CloseDesktop(desk); @@ -242,13 +210,9 @@ SBOX_TESTS_COMMAND int SwitchToSboxDesktop(int, wchar_t **) { int TestSwitchDesktop() { HDESK desktop = ::GetThreadDesktop(::GetCurrentThreadId()); - if (NULL == desktop) { + if (desktop == NULL) return SBOX_TEST_FAILED; - } - if (::SwitchDesktop(desktop)) { - return SBOX_TEST_SUCCEEDED; - } - return SBOX_TEST_DENIED; + return ::SwitchDesktop(desktop) ? SBOX_TEST_SUCCEEDED : SBOX_TEST_DENIED; } SBOX_TESTS_COMMAND int OpenAlternateDesktop(int, wchar_t **argv) { @@ -267,9 +231,8 @@ int TestOpenAlternateDesktop(wchar_t *desktop_name) { DACL_SECURITY_INFORMATION, NULL, NULL, NULL, NULL); ::CloseHandle(test_handle); - if (result == ERROR_SUCCESS) { + if (result == ERROR_SUCCESS) return SBOX_TEST_SUCCEEDED; - } } else if (::GetLastError() != ERROR_ACCESS_DENIED) { return SBOX_TEST_FAILED; } @@ -277,12 +240,10 @@ int TestOpenAlternateDesktop(wchar_t *desktop_name) { // Open by name with WRITE_DAC. desktop = ::OpenDesktop(desktop_name, 0, FALSE, WRITE_DAC); - if (desktop || ::GetLastError() != ERROR_ACCESS_DENIED) { - ::CloseDesktop(desktop); - return SBOX_TEST_SUCCEEDED; - } - - return SBOX_TEST_DENIED; + if (!desktop && ::GetLastError() == ERROR_ACCESS_DENIED) + return SBOX_TEST_DENIED; + ::CloseDesktop(desktop); + return SBOX_TEST_SUCCEEDED; } BOOL CALLBACK DesktopTestEnumProc(LPTSTR desktop_name, LPARAM result) { @@ -294,16 +255,13 @@ SBOX_TESTS_COMMAND int EnumAlternateWinsta(int, wchar_t **) { } int TestEnumAlternateWinsta() { - int result = SBOX_TEST_DENIED; // Try to enumerate the destops on the alternate windowstation. - if (::EnumDesktopsW(NULL, DesktopTestEnumProc, 0)) { - return SBOX_TEST_SUCCEEDED; - } - return SBOX_TEST_DENIED; + return ::EnumDesktopsW(NULL, DesktopTestEnumProc, 0) ? + SBOX_TEST_SUCCEEDED : SBOX_TEST_DENIED; } SBOX_TESTS_COMMAND int SleepCmd(int argc, wchar_t **argv) { - if (1 != argc) + if (argc != 1) return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND; ::Sleep(_wtoi(argv[0])); @@ -323,10 +281,8 @@ SBOX_TESTS_COMMAND int AllocateCmd(int argc, wchar_t **argv) { return SBOX_TEST_DENIED; } - if (!::VirtualFree(memory, 0, MEM_RELEASE)) - return SBOX_TEST_FAILED; - - return SBOX_TEST_SUCCEEDED; + return ::VirtualFree(memory, 0, MEM_RELEASE) ? + SBOX_TEST_SUCCEEDED : SBOX_TEST_FAILED; } diff --git a/sandbox/win/tests/validation_tests/suite.cc b/sandbox/win/tests/validation_tests/suite.cc index afb00b5..483d6fb 100644 --- a/sandbox/win/tests/validation_tests/suite.cc +++ b/sandbox/win/tests/validation_tests/suite.cc @@ -153,7 +153,6 @@ TEST(ValidationSuite, TestWindows) { TEST(ValidationSuite, TestProcessDenyLockdown) { TestRunner runner; TestRunner target; - wchar_t command[1024] = {0}; target.SetAsynchronous(true); @@ -171,7 +170,6 @@ TEST(ValidationSuite, TestProcessDenyLowIntegrity) { TestRunner runner; TestRunner target; - wchar_t command[1024] = {0}; target.SetAsynchronous(true); target.GetPolicy()->SetDelayedIntegrityLevel(INTEGRITY_LEVEL_LOW); @@ -193,7 +191,6 @@ TEST(ValidationSuite, TestProcessDenyBelowLowIntegrity) { TestRunner runner; TestRunner target; - wchar_t command[1024] = {0}; target.SetAsynchronous(true); target.GetPolicy()->SetIntegrityLevel(INTEGRITY_LEVEL_LOW); |