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}

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);