sandbox: Fix Win64 porting issue by using SIZE_T instead of ULONG

sandbox/win/src/nt_internals.h

 // Copyright 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.
 
 // This file holds definitions related to the ntdll API.
 
 #ifndef SANDBOX_WIN_SRC_NT_INTERNALS_H__
 #define SANDBOX_WIN_SRC_NT_INTERNALS_H__
 
 #include <windows.h>
@@ skipping 232 matching lines @@
 typedef struct _SECTION_BASIC_INFORMATION {
   PVOID BaseAddress;
   ULONG Attributes;
   LARGE_INTEGER Size;
 } SECTION_BASIC_INFORMATION, *PSECTION_BASIC_INFORMATION;
 
 typedef NTSTATUS (WINAPI *NtQuerySectionFunction)(
   IN HANDLE SectionHandle,
   IN SECTION_INFORMATION_CLASS SectionInformationClass,
   OUT PVOID SectionInformation,
   IN SIZE_T SectionInformationLength,

[Nico, 2015/02/16 06:58:00]

this is ULONG in dynamorio

[Will Harris, 2015/02/17 03:05:46]

sources differ on this - some say ULONG and some say SIZE_T - on balance I agree
this should be ULONG.

[Reid Kleckner, 2015/02/19 01:57:06]

Done.

   OUT PSIZE_T ReturnLength OPTIONAL);

[Nico, 2015/02/16 06:58:00]

this is PULONG in dynamorio

[Will Harris, 2015/02/17 03:05:46]

I agree this should be PULONG

[Reid Kleckner, 2015/02/19 01:57:06]

Done.

 
 // -----------------------------------------------------------------------
 // Process and Thread
 
 typedef struct _CLIENT_ID {
   PVOID UniqueProcess;
   PVOID UniqueThread;
 } CLIENT_ID, *PCLIENT_ID;
 
 typedef NTSTATUS (WINAPI *NtOpenThreadFunction) (
@@ skipping 152 matching lines @@
 
 typedef BOOLEAN (WINAPI *RtlFreeHeapFunction)(
   IN PVOID HeapHandle,
   IN ULONG Flags,
   IN PVOID HeapBase);
 
 typedef NTSTATUS (WINAPI *NtAllocateVirtualMemoryFunction) (
   IN HANDLE ProcessHandle,
   IN OUT PVOID *BaseAddress,
   IN ULONG_PTR ZeroBits,
   IN OUT PSIZE_T RegionSize,

[Nico, 2015/02/16 06:58:00]

This is a PULONG in the dynamorio code

[Will Harris, 2015/02/17 03:05:46]

I think this should remain PSIZE_T

[Reid Kleckner, 2015/02/19 01:57:06]

Left as PSIZE_T

   IN ULONG AllocationType,
   IN ULONG Protect);
 
 typedef NTSTATUS (WINAPI *NtFreeVirtualMemoryFunction) (
   IN HANDLE ProcessHandle,
   IN OUT PVOID *BaseAddress,
   IN OUT PSIZE_T RegionSize,
   IN ULONG FreeType);
 
 typedef enum _MEMORY_INFORMATION_CLASS {
   MemoryBasicInformation = 0,
   MemoryWorkingSetList,
   MemorySectionName,
   MemoryBasicVlmInformation
 } MEMORY_INFORMATION_CLASS;
 
 typedef struct _MEMORY_SECTION_NAME {  // Information Class 2
   UNICODE_STRING SectionFileName;
 } MEMORY_SECTION_NAME, *PMEMORY_SECTION_NAME;
 
 typedef NTSTATUS (WINAPI *NtQueryVirtualMemoryFunction)(
   IN HANDLE ProcessHandle,
   IN PVOID BaseAddress,
   IN MEMORY_INFORMATION_CLASS MemoryInformationClass,
   OUT PVOID MemoryInformation,
-  IN ULONG MemoryInformationLength,
-  OUT PULONG ReturnLength OPTIONAL);
+  IN SIZE_T MemoryInformationLength,
+  OUT PSIZE_T ReturnLength OPTIONAL);

[rvargas (doing something else), 2015/02/17 20:47:30]

We run elf on x64, don't we?. Which means that if this is wrong,
GetBackingModuleFilePath should be failing pretty bad.

[robertshield, 2015/02/17 23:54:43]

Yep.
Possibly. It could also accidentally work / silently fail most of the time
depending on what got passed. We have a very small number of crashes around this
code, mostly on x86 so it doesn't appear to be a case of an unnoticed kaboom.
Will look tomorrow to see if we're not getting the expected stats on x64.

[Reid Kleckner, 2015/02/19 01:57:06]

It might not fail at all if there is no useful data stored 4 bytes prior to the
variable passed in for the outparam. MSVC might just have dead data there.
chrome_elf_unittests pass with Clang in release mode, despite this overwrite.

[rvargas (doing something else), 2015/02/19 02:09:55]

My point was that looking at the code of GetBackingModuleFilePath, I'd say that
this should fail if the sizes are wrong.

So to put it another way, I don't want to change any value from 64 to 32 bits or
viceversa without someone actually making sure that the OS is really expecting
what we pass. Given that we are using some of this code, I'm inclined to say
that it works unless proven wrong.

Note that for the first four arguments we have kind of a free pass as they go in
registers (so the result depends on whatever was in the high part), but after
that, sizes should match the stack use (I think).

 
 typedef NTSTATUS (WINAPI *NtProtectVirtualMemoryFunction)(
   IN HANDLE ProcessHandle,
   IN OUT PVOID* BaseAddress,
   IN OUT PSIZE_T ProtectSize,

[Nico, 2015/02/16 06:58:00]

This is a PULONG in dynamorio

[Will Harris, 2015/02/17 03:05:46]

I think this should remain PSIZE_T

[Reid Kleckner, 2015/02/19 01:57:06]

Left as PSIZE_T

   IN ULONG NewProtect,
   OUT PULONG OldProtect);
 
 // -----------------------------------------------------------------------
 // Objects
 
 typedef enum _OBJECT_INFORMATION_CLASS {
   ObjectBasicInformation,
   ObjectNameInformation,
   ObjectTypeInformation,
@@ skipping 181 matching lines @@
     POBJECT_ATTRIBUTES ObjectAttributes);
 
 #define DIRECTORY_QUERY               0x0001
 #define DIRECTORY_TRAVERSE            0x0002
 #define DIRECTORY_CREATE_OBJECT       0x0004
 #define DIRECTORY_CREATE_SUBDIRECTORY 0x0008
 #define DIRECTORY_ALL_ACCESS          0x000F
 
 #endif  // SANDBOX_WIN_SRC_NT_INTERNALS_H__