Fixes to possible GetLastError bugs

sandbox/win/src/process_thread_interception.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sandbox/win/src/process_thread_interception.h"
 
 #include "sandbox/win/src/crosscall_client.h"
 #include "sandbox/win/src/ipc_tags.h"
 #include "sandbox/win/src/policy_params.h"
 #include "sandbox/win/src/policy_target.h"
@@ skipping 257 matching lines @@
                                  LPSTARTUPINFOW startup_info,
                                  LPPROCESS_INFORMATION process_information) {
   if (SandboxFactory::GetTargetServices()->GetState()->IsCsrssConnected() &&
       orig_CreateProcessW(application_name, command_line, process_attributes,
                           thread_attributes, inherit_handles, flags,
                           environment, current_directory, startup_info,
                           process_information)) {
     return TRUE;
   }
 
-  // We don't trust that the IPC can work this early.
-  if (!SandboxFactory::GetTargetServices()->GetState()->InitCalled())
-    return FALSE;
-
   DWORD original_error = ::GetLastError();
 
-  do {
-    if (!ValidParameter(process_information, sizeof(PROCESS_INFORMATION),
-                        WRITE))
-      break;
+  // We don't trust that the IPC can work this early.
+  if (SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {

[brucedawson, 2015/09/12 00:33:55]

Moving this below the initialization of original_error because I can't tell
whether it calls anything that could perturb LastError.

[cpu_(ooo_6.6-7.5), 2015/09/14 17:17:16]

In this particular case it is harmless to move it but in general we should keep
the interception code as it was. Reason being we can reach a TargetXXX function
before anything but ntdll has properly appeared in our process, including
kernel32.

So the answer is InitCalled() is just a memory check, so lets leave as it was.

[brucedawson, 2015/09/14 18:59:55]

Got it.

How about a comment to make the rational clearer. Possible comment added.

+    do {
+      if (!ValidParameter(process_information, sizeof(PROCESS_INFORMATION),
+                          WRITE))
+        break;
 
-    void* memory = GetGlobalIPCMemory();
-    if (NULL == memory)
-      break;
+      void* memory = GetGlobalIPCMemory();
+      if (NULL == memory)
+        break;
 
-    const wchar_t* cur_dir = NULL;
+      const wchar_t* cur_dir = NULL;
 
-    wchar_t current_directory[MAX_PATH];
-    DWORD result = ::GetCurrentDirectory(MAX_PATH, current_directory);
-    if (0 != result && result < MAX_PATH)
-      cur_dir = current_directory;
+      wchar_t current_directory[MAX_PATH];
+      DWORD result = ::GetCurrentDirectory(MAX_PATH, current_directory);
+      if (0 != result && result < MAX_PATH)
+        cur_dir = current_directory;
 
-    SharedMemIPCClient ipc(memory);
-    CrossCallReturn answer = {0};
+      SharedMemIPCClient ipc(memory);
+      CrossCallReturn answer = {0};
 
-    InOutCountedBuffer proc_info(process_information,
-                                 sizeof(PROCESS_INFORMATION));
+      InOutCountedBuffer proc_info(process_information,
+                                   sizeof(PROCESS_INFORMATION));
 
-    ResultCode code = CrossCall(ipc, IPC_CREATEPROCESSW_TAG, application_name,
-                                command_line, cur_dir, proc_info, &answer);
-    if (SBOX_ALL_OK != code)
-      break;
+      ResultCode code = CrossCall(ipc, IPC_CREATEPROCESSW_TAG, application_name,
+                                  command_line, cur_dir, proc_info, &answer);
+      if (SBOX_ALL_OK != code)
+        break;
 
-    ::SetLastError(answer.win32_result);
-    if (ERROR_SUCCESS != answer.win32_result)
-      return FALSE;
+      ::SetLastError(answer.win32_result);
+      if (ERROR_SUCCESS != answer.win32_result)
+        return FALSE;
 
-    return TRUE;
-  } while (false);
+      return TRUE;
+    } while (false);
+  }
 
   ::SetLastError(original_error);
   return FALSE;
 }
 
 BOOL WINAPI TargetCreateProcessA(CreateProcessAFunction orig_CreateProcessA,
                                  LPCSTR application_name, LPSTR command_line,
                                  LPSECURITY_ATTRIBUTES process_attributes,
                                  LPSECURITY_ATTRIBUTES thread_attributes,
                                  BOOL inherit_handles, DWORD flags,
                                  LPVOID environment, LPCSTR current_directory,
                                  LPSTARTUPINFOA startup_info,
                                  LPPROCESS_INFORMATION process_information) {
   if (orig_CreateProcessA(application_name, command_line, process_attributes,
                           thread_attributes, inherit_handles, flags,
                           environment, current_directory, startup_info,
                           process_information)) {
     return TRUE;
   }
 
-  // We don't trust that the IPC can work this early.
-  if (!SandboxFactory::GetTargetServices()->GetState()->InitCalled())
-    return FALSE;
-
   DWORD original_error = ::GetLastError();
 
-  do {
-    if (!ValidParameter(process_information, sizeof(PROCESS_INFORMATION),
-                        WRITE))
-      break;
+  // We don't trust that the IPC can work this early.
+  if (SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
+    do {
+      if (!ValidParameter(process_information, sizeof(PROCESS_INFORMATION),
+                          WRITE))
+        break;
 
-    void* memory = GetGlobalIPCMemory();
-    if (NULL == memory)
-      break;
+      void* memory = GetGlobalIPCMemory();
+      if (NULL == memory)
+        break;
 
-    // Convert the input params to unicode.
-    UNICODE_STRING *cmd_unicode = NULL;
-    UNICODE_STRING *app_unicode = NULL;
-    if (command_line) {
-      cmd_unicode = AnsiToUnicode(command_line);
-      if (!cmd_unicode)
+      // Convert the input params to unicode.
+      UNICODE_STRING *cmd_unicode = NULL;
+      UNICODE_STRING *app_unicode = NULL;
+      if (command_line) {
+        cmd_unicode = AnsiToUnicode(command_line);
+        if (!cmd_unicode)
+          break;
+      }
+
+      if (application_name) {
+        app_unicode = AnsiToUnicode(application_name);
+        if (!app_unicode) {
+          operator delete(cmd_unicode, NT_ALLOC);
+          break;
+        }
+      }
+
+      const wchar_t* cmd_line = cmd_unicode ? cmd_unicode->Buffer : NULL;
+      const wchar_t* app_name = app_unicode ? app_unicode->Buffer : NULL;
+      const wchar_t* cur_dir = NULL;
+
+      wchar_t current_directory[MAX_PATH];
+      DWORD result = ::GetCurrentDirectory(MAX_PATH, current_directory);
+      if (0 != result && result < MAX_PATH)
+        cur_dir = current_directory;
+
+      SharedMemIPCClient ipc(memory);
+      CrossCallReturn answer = {0};
+
+      InOutCountedBuffer proc_info(process_information,
+                                   sizeof(PROCESS_INFORMATION));
+
+      ResultCode code = CrossCall(ipc, IPC_CREATEPROCESSW_TAG, app_name,
+                                  cmd_line, cur_dir, proc_info, &answer);
+
+      operator delete(cmd_unicode, NT_ALLOC);
+      operator delete(app_unicode, NT_ALLOC);
+
+      if (SBOX_ALL_OK != code)
         break;
-    }
 
-    if (application_name) {
-      app_unicode = AnsiToUnicode(application_name);
-      if (!app_unicode) {
-        operator delete(cmd_unicode, NT_ALLOC);
-        break;
-      }
-    }
+      ::SetLastError(answer.win32_result);
+      if (ERROR_SUCCESS != answer.win32_result)
+        return FALSE;
 
-    const wchar_t* cmd_line = cmd_unicode ? cmd_unicode->Buffer : NULL;
-    const wchar_t* app_name = app_unicode ? app_unicode->Buffer : NULL;
-    const wchar_t* cur_dir = NULL;
-
-    wchar_t current_directory[MAX_PATH];
-    DWORD result = ::GetCurrentDirectory(MAX_PATH, current_directory);
-    if (0 != result && result < MAX_PATH)
-      cur_dir = current_directory;
-
-    SharedMemIPCClient ipc(memory);
-    CrossCallReturn answer = {0};
-
-    InOutCountedBuffer proc_info(process_information,
-                                 sizeof(PROCESS_INFORMATION));
-
-    ResultCode code = CrossCall(ipc, IPC_CREATEPROCESSW_TAG, app_name,
-                                cmd_line, cur_dir, proc_info, &answer);
-
-    operator delete(cmd_unicode, NT_ALLOC);
-    operator delete(app_unicode, NT_ALLOC);
-
-    if (SBOX_ALL_OK != code)
-      break;
-
-    ::SetLastError(answer.win32_result);
-    if (ERROR_SUCCESS != answer.win32_result)
-      return FALSE;
-
-    return TRUE;
-  } while (false);
+      return TRUE;
+    } while (false);
+  }
 
   ::SetLastError(original_error);
   return FALSE;
 }
 
 }  // namespace sandbox