Restructure sandbox code to reduce dependencies pulled in by intercept code.

sandbox/win/src/sandbox_nt_util.cc

 // Copyright (c) 2012 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/sandbox_nt_util.h"
 
 #include "base/win/pe_image.h"
 #include "sandbox/win/src/sandbox_factory.h"
 #include "sandbox/win/src/target_services.h"
 
 namespace sandbox {
 
 // This is the list of all imported symbols from ntdll.dll.
-SANDBOX_INTERCEPT NtExports g_nt = { NULL };
+SANDBOX_INTERCEPT NtExports g_nt;
 
 }  // namespace sandbox
 
 namespace {
 
 #if defined(_WIN64)
 void* AllocateNearTo(void* source, size_t size) {
   using sandbox::g_nt;
 
   // Start with 1 GB above the source.
@@ skipping 72 matching lines @@
   if (!NT_SUCCESS(ret))
     return NULL;
   return base;
 }
 #endif  // defined(_WIN64).
 
 }  // namespace.
 
 namespace sandbox {
 
-// Handle for our private heap.
-void* g_heap = NULL;
+// This is the list of all imported symbols from ntdll.dll.
+SANDBOX_INTERCEPT NtExports g_nt;

[rvargas (doing something else), 2013/11/27 23:53:51]

This is already defined up there

[robertshield, 2013/11/29 01:21:26]

Done.

 
 SANDBOX_INTERCEPT HANDLE g_shared_section;
 SANDBOX_INTERCEPT size_t g_shared_IPC_size = 0;
 SANDBOX_INTERCEPT size_t g_shared_policy_size = 0;
 
 void* volatile g_shared_policy_memory = NULL;
 void* volatile g_shared_IPC_memory = NULL;
 
 // Both the IPC and the policy share a single region of memory in which the IPC
 // memory is first and the policy memory is last.
@@ skipping 31 matching lines @@
     return NULL;
   return g_shared_IPC_memory;
 }
 
 void* GetGlobalPolicyMemory() {
   if (!MapGlobalMemory())
     return NULL;
   return g_shared_policy_memory;
 }
 
+// Handle for our private heap.
+void* g_heap = NULL;

[rvargas (doing something else), 2013/11/27 23:53:51]

don't move this to the middle of the file. The common pracatice is to have
global definitions at the top of the file.

[robertshield, 2013/11/29 01:21:26]

Done.

+
 bool InitHeap() {
   if (!g_heap) {
     // Create a new heap using default values for everything.
     void* heap = g_nt.RtlCreateHeap(HEAP_GROWABLE, NULL, 0, 0, NULL, NULL);
     if (!heap)
       return false;
 
     if (NULL != _InterlockedCompareExchangePointer(&g_heap, heap, NULL)) {
       // Somebody beat us to the memory setup.
       g_nt.RtlDestroyHeap(heap);
@@ skipping 28 matching lines @@
 bool ValidParameter(void* buffer, size_t size, RequiredAccess intent) {
   DCHECK_NT(size);
   __try {
     TouchMemory(buffer, size, intent);
   } __except(EXCEPTION_EXECUTE_HANDLER) {
     return false;
   }
   return true;
 }
 
-NTSTATUS CopyData(void* destination, const void* source, size_t bytes) {

[rvargas (doing something else), 2013/11/27 23:53:51]

I presume this is being moved to nt_util_base. I have to look at the rest, but
it is weird to say that interceptions now don't have access to nt_utils, and
they only have access to nt_utils_base, especially since there is nothing wrong
with ValidParameter() or any other of the functions in this file.

I hear what you say about these two functions bringing in other stuff, but I
don't think the solution is to move them apart so that the patching code is free
from them. From a logical point of view, interceptors are more restricted than
the code that performs the patching, so if they are no good for patching they
are no good for interceptors either.

But of course they are needed for the interceptors, and that means that they
need to be fixed by removing the dependency or hiding it (but not hiding the
method itself)

[rvargas (doing something else), 2013/11/27 23:53:51]

After looking at the whole patch I see that interceptors have access to nt_util
and nt_util_base, and that is pretty much the same that I said about having a
cached pointer for TargetServicesBase::GetInstance (because some cc has to
initialize it from an interceptor if it is null).

So this is probably fine, but then the name is misleading, as in  *_base is not
any more simpler, basic or common than the rest of nt_util.

Maybe something that reflects the fact that the methods there depend on
TargetServices.

I really hate moving AllocAndCopyName and CopyData just to get memcpy... all we
need is a bool, or a function pointer to call.

[rvargas (doing something else), 2013/11/27 23:58:52]

There is nt!memcpy (at least on Win7 but i expect everywhere). Maybe just add
that to g_nt

[robertshield, 2013/11/29 01:21:26]

Done.

[robertshield, 2013/11/29 01:21:26]

Done.

[robertshield, 2013/11/29 01:21:26]

And this solves all known problems. Using nt's memcpy removes the dependency on
target_services.obj which allows the sandbox_nt_util_base stuff to go away.
Hooray!

-  NTSTATUS ret = STATUS_SUCCESS;
-  __try {
-    if (SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
-      memcpy(destination, source, bytes);
-    } else {
-      const char* from = reinterpret_cast<const char*>(source);
-      char* to = reinterpret_cast<char*>(destination);
-      for (size_t i = 0; i < bytes; i++) {
-        to[i] = from[i];
-      }
-    }
-  } __except(EXCEPTION_EXECUTE_HANDLER) {
-    ret = GetExceptionCode();
-  }
-  return ret;
-}
-
-// Hacky code... replace with AllocAndCopyObjectAttributes.
-NTSTATUS AllocAndCopyName(const OBJECT_ATTRIBUTES* in_object,
-                          wchar_t** out_name, uint32* attributes,
-                          HANDLE* root) {
-  if (!InitHeap())
-    return STATUS_NO_MEMORY;
-
-  DCHECK_NT(out_name);
-  *out_name = NULL;
-  NTSTATUS ret = STATUS_UNSUCCESSFUL;
-  __try {
-    do {
-      if (in_object->RootDirectory != static_cast<HANDLE>(0) && !root)
-        break;
-      if (NULL == in_object->ObjectName)
-        break;
-      if (NULL == in_object->ObjectName->Buffer)
-        break;
-
-      size_t size = in_object->ObjectName->Length + sizeof(wchar_t);
-      *out_name = new(NT_ALLOC) wchar_t[size/sizeof(wchar_t)];
-      if (NULL == *out_name)
-        break;
-
-      ret = CopyData(*out_name, in_object->ObjectName->Buffer,
-                     size - sizeof(wchar_t));
-      if (!NT_SUCCESS(ret))
-        break;
-
-      (*out_name)[size / sizeof(wchar_t) - 1] = L'\0';
-
-      if (attributes)
-        *attributes = in_object->Attributes;
-
-      if (root)
-        *root = in_object->RootDirectory;
-      ret = STATUS_SUCCESS;
-    } while (false);
-  } __except(EXCEPTION_EXECUTE_HANDLER) {
-    ret = GetExceptionCode();
-  }
-
-  if (!NT_SUCCESS(ret) && *out_name) {
-    operator delete(*out_name, NT_ALLOC);
-    *out_name = NULL;
-  }
-
-  return ret;
-}
-
 NTSTATUS GetProcessId(HANDLE process, ULONG *process_id) {
   PROCESS_BASIC_INFORMATION proc_info;
   ULONG bytes_returned;
 
   NTSTATUS ret = g_nt.QueryInformationProcess(process, ProcessBasicInformation,
                                               &proc_info, sizeof(proc_info),
                                               &bytes_returned);
   if (!NT_SUCCESS(ret) || sizeof(proc_info) != bytes_returned)
     return ret;
 
@@ skipping 138 matching lines @@
     }
     if (!NT_SUCCESS(ret)) {
       operator delete(section_name, NT_ALLOC);
       return NULL;
     }
 
     return reinterpret_cast<UNICODE_STRING*>(section_name);
   }
 }
 
-UNICODE_STRING* ExtractModuleName(const UNICODE_STRING* module_path) {
-  if ((!module_path) || (!module_path->Buffer))
-    return NULL;
-
-  wchar_t* sep = NULL;
-  int start_pos = module_path->Length / sizeof(wchar_t) - 1;
-  int ix = start_pos;
-
-  for (; ix >= 0; --ix) {
-    if (module_path->Buffer[ix] == L'\\') {
-      sep = &module_path->Buffer[ix];
-      break;
-    }
-  }
-
-  // Ends with path separator. Not a valid module name.
-  if ((ix == start_pos) && sep)
-    return NULL;
-
-  // No path separator found. Use the entire name.
-  if (!sep) {
-    sep = &module_path->Buffer[-1];
-  }
-
-  // Add one to the size so we can null terminate the string.
-  size_t size_bytes = (start_pos - ix + 1) * sizeof(wchar_t);
-
-  // Based on the code above, size_bytes should always be small enough
-  // to make the static_cast below safe.
-  DCHECK_NT(kuint16max > size_bytes);
-  char* str_buffer = new(NT_ALLOC) char[size_bytes + sizeof(UNICODE_STRING)];
-  if (!str_buffer)
-    return NULL;
-
-  UNICODE_STRING* out_string = reinterpret_cast<UNICODE_STRING*>(str_buffer);
-  out_string->Buffer = reinterpret_cast<wchar_t*>(&out_string[1]);
-  out_string->Length = static_cast<USHORT>(size_bytes - sizeof(wchar_t));
-  out_string->MaximumLength = static_cast<USHORT>(size_bytes);
-
-  NTSTATUS ret = CopyData(out_string->Buffer, &sep[1], out_string->Length);
-  if (!NT_SUCCESS(ret)) {
-    operator delete(out_string, NT_ALLOC);
-    return NULL;
-  }
-
-  out_string->Buffer[out_string->Length / sizeof(wchar_t)] = L'\0';
-  return out_string;
-}
-
 NTSTATUS AutoProtectMemory::ChangeProtection(void* address, size_t bytes,
                                              ULONG protect) {
   DCHECK_NT(!changed_);
   SIZE_T new_bytes = bytes;
   NTSTATUS ret = g_nt.ProtectVirtualMemory(NtCurrentProcess, &address,
                                            &new_bytes, protect, &old_protect_);
   if (NT_SUCCESS(ret)) {
     changed_ = true;
     address_ = address;
     bytes_ = new_bytes;
@@ skipping 100 matching lines @@
   UNREFERENCED_PARAMETER(type);
   return buffer;
 }
 
 void __cdecl operator delete(void* memory, void* buffer,
                              sandbox::AllocationType type) {
   UNREFERENCED_PARAMETER(memory);
   UNREFERENCED_PARAMETER(buffer);
   UNREFERENCED_PARAMETER(type);
 }