Fixes to possible GetLastError bugs

sandbox/win/src/process_thread_interception.cc

Index: sandbox/win/src/process_thread_interception.cc
diff --git a/sandbox/win/src/process_thread_interception.cc b/sandbox/win/src/process_thread_interception.cc
index e6c8c2e9180c91b96164791b67de1081998b2be4..e8e7b353e3f9561099eb18c48e9eae8736785193 100644
--- a/sandbox/win/src/process_thread_interception.cc
+++ b/sandbox/win/src/process_thread_interception.cc
@@ -275,45 +275,44 @@ BOOL WINAPI TargetCreateProcessW(CreateProcessWFunction orig_CreateProcessW,
     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;
@@ -334,68 +333,67 @@ BOOL WINAPI TargetCreateProcessA(CreateProcessAFunction orig_CreateProcessA,
     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;
-
-    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)
+  // 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;
-    }
 
-    if (application_name) {
-      app_unicode = AnsiToUnicode(application_name);
-      if (!app_unicode) {
-        operator delete(cmd_unicode, NT_ALLOC);
+      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)
+          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;
+      if (application_name) {
+        app_unicode = AnsiToUnicode(application_name);
+        if (!app_unicode) {
+          operator delete(cmd_unicode, NT_ALLOC);
+          break;
+        }
+      }
 
-    wchar_t current_directory[MAX_PATH];
-    DWORD result = ::GetCurrentDirectory(MAX_PATH, current_directory);
-    if (0 != result && result < MAX_PATH)
-      cur_dir = current_directory;
+      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;
 
-    SharedMemIPCClient ipc(memory);
-    CrossCallReturn answer = {0};
+      wchar_t current_directory[MAX_PATH];
+      DWORD result = ::GetCurrentDirectory(MAX_PATH, current_directory);
+      if (0 != result && result < MAX_PATH)
+        cur_dir = current_directory;
 
-    InOutCountedBuffer proc_info(process_information,
-                                 sizeof(PROCESS_INFORMATION));
+      SharedMemIPCClient ipc(memory);
+      CrossCallReturn answer = {0};
 
-    ResultCode code = CrossCall(ipc, IPC_CREATEPROCESSW_TAG, app_name,
-                                cmd_line, cur_dir, proc_info, &answer);
+      InOutCountedBuffer proc_info(process_information,
+                                   sizeof(PROCESS_INFORMATION));
 
-    operator delete(cmd_unicode, NT_ALLOC);
-    operator delete(app_unicode, NT_ALLOC);
+      ResultCode code = CrossCall(ipc, IPC_CREATEPROCESSW_TAG, app_name,
+                                  cmd_line, cur_dir, proc_info, &answer);
 
-    if (SBOX_ALL_OK != code)
-      break;
+      operator delete(cmd_unicode, NT_ALLOC);
+      operator delete(app_unicode, NT_ALLOC);
 
-    ::SetLastError(answer.win32_result);
-    if (ERROR_SUCCESS != answer.win32_result)
-      return FALSE;
+      if (SBOX_ALL_OK != code)
+        break;
 
-    return TRUE;
-  } while (false);
+      ::SetLastError(answer.win32_result);
+      if (ERROR_SUCCESS != answer.win32_result)
+        return FALSE;
+
+      return TRUE;
+    } while (false);
+  }
 
   ::SetLastError(original_error);
   return FALSE;