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

sandbox/win/src/sandbox_nt_util.cc

Index: sandbox/win/src/sandbox_nt_util.cc
diff --git a/sandbox/win/src/sandbox_nt_util.cc b/sandbox/win/src/sandbox_nt_util.cc
index 71314611283e85057964617b191b3514f18b964e..88805a23d22599e17a331ffeb62dd9cb3e13a2fb 100644
--- a/sandbox/win/src/sandbox_nt_util.cc
+++ b/sandbox/win/src/sandbox_nt_util.cc
@@ -11,7 +11,7 @@
 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
 
@@ -104,8 +104,8 @@ void* AllocateNearTo(void* source, size_t size) {
 
 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;
@@ -157,6 +157,9 @@ void* GetGlobalPolicyMemory() {
   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.
@@ -205,74 +208,6 @@ bool ValidParameter(void* buffer, size_t size, RequiredAccess intent) {
   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;
@@ -431,55 +366,6 @@ UNICODE_STRING* GetBackingFilePath(PVOID address) {
   }
 }
 
-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_);