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

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

BUG=322710
TEST=NONE

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238538

[rvargas (doing something else), 2013-11-26 20:42:11]

Maybe the whole issue is logging?

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/filesyste...
File sandbox/win/src/filesystem_interception.cc (right):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/filesyste...
sandbox/win/src/filesystem_interception.cc:13: #include
"sandbox/win/src/sandbox_utils.h"
Interception code cannot depend on sandbox_utils... there is kernel32 code there

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/policy_ta...
File sandbox/win/src/policy_target.cc (right):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/policy_ta...
sandbox/win/src/policy_target.cc:14: #include "sandbox/win/src/sandbox_utils.h"
same here

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:119: bool MapGlobalMemory() {
I don't see anything in this block of code that should cause a dependency from
base. What am I missing?

This should be usable directly from nt code (without kernel32) because it is
called from nt interceptions. If that is not the case, it has to be fixed :(

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:211: if
(SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
Same thing here... and ExtractModuleName... is it the issue that it calls
GetTargetServices? We should be able to provide that without a real
implementation of the rest of the sandbox in case that it matters, but all
external dependencies boil down to variables in memory that are supposed to be
set up by the parent process.

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (right):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:386: if (!::VirtualProtectEx(child_process,
address, length,
This, on the other hand, is not part of the nt layer (depends on kernel32) so it
cannot be here.

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
File sandbox/win/src/service_resolver.cc (left):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver.cc:27: DCHECK(address);
removing this is fine

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver.cc:35: NOTREACHED();
but this is a little more worrisome. How about DCHECK_NT(false)?

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
File sandbox/win/src/service_resolver_32.cc (left):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver_32.cc:8: #include "sandbox/win/src/win_utils.h"
Looks like the only thing from win_utils that requires an object file from base
is logging. And even that, we only use CHECK, DCHECK and NOTREACHED. Would
removing base logging be enough? We could move the macros from nt_utils to a new
header usable from both sides of the sandbox (nt and not-nt), at least from
these files, and remove logging.h.

Or simply include nt_utils just for the check macros.

[robertshield, 2013-11-27 20:28:00]

Thanks Ricardo, reworked things into the nt-only vs not-nt-only division. 

The logging hint was really helpful, that cleans things up a lot. 

The two remaining issues were use of an extern var in sandbox_nt_util.cc that
was defined in sandbox.cc. I moved that var into its own compilation unit (which
is a good idea anyway). 

The last thing is the CopyData function and its callers that I split into
sandbox_nt_util_base.h.

PTAL!

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/filesyste...
File sandbox/win/src/filesystem_interception.cc (right):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/filesyste...
sandbox/win/src/filesystem_interception.cc:13: #include
"sandbox/win/src/sandbox_utils.h"
On 2013/11/26 20:42:12, rvargas wrote:
> Interception code cannot depend on sandbox_utils... there is kernel32 code
there

Done.

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/policy_ta...
File sandbox/win/src/policy_target.cc (right):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/policy_ta...
sandbox/win/src/policy_target.cc:14: #include "sandbox/win/src/sandbox_utils.h"
On 2013/11/26 20:42:12, rvargas wrote:
> same here

Done.

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:119: bool MapGlobalMemory() {
On 2013/11/26 20:42:12, rvargas wrote:
> I don't see anything in this block of code that should cause a dependency from
> base. What am I missing?

The code in just this block doesn't actually pull in base (that was another
bit), but it does pull in some user32 symbols (__imp__GetDesktopWindow,
__imp__CloseWindowStation) etc. 

This happens because of the g_shared_section vars above which is defined in
another compilation unit (sandbox.obj) which itself has user32 dependencies. 

Found that a nice fix to this is to have the globals that are shared between
compilation units be defined in their own cu that itself incurs no extra
dependencies, so added sandbox_globals.cc. Let me know what you think.


> 
> This should be usable directly from nt code (without kernel32) because it is
> called from nt interceptions. If that is not the case, it has to be fixed :(

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:211: if
(SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
On 2013/11/26 20:42:12, rvargas wrote:
> Same thing here... and ExtractModuleName... is it the issue that it calls
> GetTargetServices? We should be able to provide that without a real
> implementation of the rest of the sandbox in case that it matters, but all
> external dependencies boil down to variables in memory that are supposed to be
> set up by the parent process.

Yes, the GetTargetServices() call is the problem. That causes sandbox.obj to be
preserved and that carries with it a bunch of depdendencies. I've split this
code out into sandbox_nt_util_base.cc which solves this a bit more nicely than
the previous patch.

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (right):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:386: if (!::VirtualProtectEx(child_process,
address, length,
On 2013/11/26 20:42:12, rvargas wrote:
> This, on the other hand, is not part of the nt layer (depends on kernel32) so
it
> cannot be here.

Moved it back.

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
File sandbox/win/src/service_resolver.cc (left):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver.cc:27: DCHECK(address);
On 2013/11/26 20:42:12, rvargas wrote:
> removing this is fine

Done.

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver.cc:35: NOTREACHED();
On 2013/11/26 20:42:12, rvargas wrote:
> but this is a little more worrisome. How about DCHECK_NT(false)?

Done.

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
File sandbox/win/src/service_resolver_32.cc (left):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver_32.cc:8: #include "sandbox/win/src/win_utils.h"
On 2013/11/26 20:42:12, rvargas wrote:
> Looks like the only thing from win_utils that requires an object file from
base
> is logging. And even that, we only use CHECK, DCHECK and NOTREACHED. Would
> removing base logging be enough? We could move the macros from nt_utils to a
new
> header usable from both sides of the sandbox (nt and not-nt), at least from
> these files, and remove logging.h.
> 
> Or simply include nt_utils just for the check macros.

Ah, that's a much better idea that what I did :) Done as you suggest.

[rvargas (doing something else), 2013-11-27 23:53:50]

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:211: if
(SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
On 2013/11/27 20:28:01, robertshield wrote:
> On 2013/11/26 20:42:12, rvargas wrote:
> > Same thing here... and ExtractModuleName... is it the issue that it calls
> > GetTargetServices? We should be able to provide that without a real
> > implementation of the rest of the sandbox in case that it matters, but all
> > external dependencies boil down to variables in memory that are supposed to
be
> > set up by the parent process.
> 
> Yes, the GetTargetServices() call is the problem. That causes sandbox.obj to
be
> preserved and that carries with it a bunch of depdendencies. I've split this
> code out into sandbox_nt_util_base.cc which solves this a bit more nicely than
> the previous patch.

Carlos may have an opinion here, but I'd say we could either have a cached
pointer somewhere (instead of going into TargetServicesBase::GetInstance()
everytime), or split target_services into methods that depend on kerenl/user and
methods that don't.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/sandbox_win.gypi
File sandbox/win/sandbox_win.gypi (right):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/sandbox_win.g...
sandbox/win/sandbox_win.gypi:96: 'src/sandbox_globals.cc',
missing files

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:208: NTSTATUS CopyData(void* destination,
const void* source, size_t bytes) {
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)

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:208: NTSTATUS CopyData(void* destination,
const void* source, size_t bytes) {
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.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (right):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:108: SANDBOX_INTERCEPT NtExports g_nt;
This is already defined up there

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:161: void* g_heap = NULL;
don't move this to the middle of the file. The common pracatice is to have
global definitions at the top of the file.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_u...
File sandbox/win/src/sandbox_utils.cc (left):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_u...
sandbox/win/src/sandbox_utils.cc:7: #include <windows.h>
why?

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
File sandbox/win/src/service_resolver.cc (right):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver.cc:41: void
ServiceResolverThunk::AllowLocalPatches() {
This requires unit tests

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
File sandbox/win/src/service_resolver_64.cc (left):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver_64.cc:154: DCHECK_GE(GetInternalThunkSize(),
sizeof(local_service));
Convert DCHECKS to appropriate DCHECK_NT

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver_64.cc:184: NOTREACHED();
Don't remove these

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/win_utils.cc
File sandbox/win/src/win_utils.cc (right):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/win_utils...
sandbox/win/src/win_utils.cc:322: VERIFY(*function_ptr);
We need CHECK_NT

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

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:208: NTSTATUS CopyData(void* destination,
const void* source, size_t bytes) {
There is nt!memcpy (at least on Win7 but i expect everywhere). Maybe just add
that to g_nt

[robertshield, 2013-11-28 01:39:19]

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/sandbox_win.gypi
File sandbox/win/sandbox_win.gypi (right):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/sandbox_win.g...
sandbox/win/sandbox_win.gypi:96: 'src/sandbox_globals.cc',
On 2013/11/27 23:53:51, rvargas wrote:
> missing files

ugh, sorry about that. missing files added, going over the other comments now.

[robertshield, 2013-11-29 01:21:25]

Thanks, PTAL!

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/20001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:211: if
(SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {
On 2013/11/27 23:53:50, rvargas wrote:
> On 2013/11/27 20:28:01, robertshield wrote:
> > On 2013/11/26 20:42:12, rvargas wrote:
> > > Same thing here... and ExtractModuleName... is it the issue that it calls
> > > GetTargetServices? We should be able to provide that without a real
> > > implementation of the rest of the sandbox in case that it matters, but all
> > > external dependencies boil down to variables in memory that are supposed
to
> be
> > > set up by the parent process.
> > 
> > Yes, the GetTargetServices() call is the problem. That causes sandbox.obj to
> be
> > preserved and that carries with it a bunch of depdendencies. I've split this
> > code out into sandbox_nt_util_base.cc which solves this a bit more nicely
than
> > the previous patch.
> 
> Carlos may have an opinion here, but I'd say we could either have a cached
> pointer somewhere (instead of going into TargetServicesBase::GetInstance()
> everytime), or split target_services into methods that depend on kerenl/user
and
> methods that don't.

I believe this is now solved by your observation that we can use ntdll's memcpy.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:208: NTSTATUS CopyData(void* destination,
const void* source, size_t bytes) {
On 2013/11/27 23:53:51, rvargas wrote:
> 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)

Done.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:208: NTSTATUS CopyData(void* destination,
const void* source, size_t bytes) {
On 2013/11/27 23:53:51, rvargas wrote:
> 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.

Done.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:208: NTSTATUS CopyData(void* destination,
const void* source, size_t bytes) {
On 2013/11/27 23:58:52, rvargas wrote:
> There is nt!memcpy (at least on Win7 but i expect everywhere). Maybe just add
> that to g_nt

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!

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
File sandbox/win/src/sandbox_nt_util.cc (right):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:108: SANDBOX_INTERCEPT NtExports g_nt;
On 2013/11/27 23:53:51, rvargas wrote:
> This is already defined up there

Done.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_n...
sandbox/win/src/sandbox_nt_util.cc:161: void* g_heap = NULL;
On 2013/11/27 23:53:51, rvargas wrote:
> don't move this to the middle of the file. The common pracatice is to have
> global definitions at the top of the file.

Done.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_u...
File sandbox/win/src/sandbox_utils.cc (left):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/sandbox_u...
sandbox/win/src/sandbox_utils.cc:7: #include <windows.h>
On 2013/11/27 23:53:51, rvargas wrote:
> why?

earlier refactoring cruft. undone.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
File sandbox/win/src/service_resolver.cc (right):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver.cc:41: void
ServiceResolverThunk::AllowLocalPatches() {
On 2013/11/27 23:53:51, rvargas wrote:
> This requires unit tests

Test added.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
File sandbox/win/src/service_resolver_64.cc (left):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver_64.cc:154: DCHECK_GE(GetInternalThunkSize(),
sizeof(local_service));
On 2013/11/27 23:53:51, rvargas wrote:
> Convert DCHECKS to appropriate DCHECK_NT

Done.

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/service_r...
sandbox/win/src/service_resolver_64.cc:184: NOTREACHED();
On 2013/11/27 23:53:51, rvargas wrote:
> Don't remove these

Replaced with DCHECK_NT(false);

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/win_utils.cc
File sandbox/win/src/win_utils.cc (right):

https://codereview.chromium.org/84063004/diff/80001/sandbox/win/src/win_utils...
sandbox/win/src/win_utils.cc:322: VERIFY(*function_ptr);
On 2013/11/27 23:53:51, rvargas wrote:
> We need CHECK_NT

Done.

[cpu_(ooo_6.6-7.5), 2013-11-29 18:30:07]

looking good.

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_nt_util.cc:216: for (size_t i = 0; i < bytes; i++) {
nice

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver.cc (right):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
sandbox/win/src/service_resolver.cc:42: ntdll_base_ =
::GetModuleHandle(L"ntdll.dll");
don't we have a constant for "ntdll.dll"? if it is too cumbersome we can leave
it as is.

Btw, I believe that (contrary to most of win32) the loader functions that take
strings are natively single byte strings (ansi?) so calling GetModuleHandleA is
better.

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver.h (right):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
sandbox/win/src/service_resolver.h:47: virtual void AllowLocalPatches();
Do we want a way to unset this? 

I am correct in thinking we don't run into issues because this lives in the elf
dll vs trying to autopatch AND being the broker in chrome.exe ?

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver_unittest.cc (right):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
sandbox/win/src/service_resolver_unittest.cc:262: &used);
indenting

[robertshield, 2013-11-29 19:29:08]

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_nt_util.cc (left):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_nt_util.cc:216: for (size_t i = 0; i < bytes; i++) {
On 2013/11/29 18:30:07, cpu wrote:
> nice

Ricardo's idea :)

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver.cc (right):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
sandbox/win/src/service_resolver.cc:42: ntdll_base_ =
::GetModuleHandle(L"ntdll.dll");
On 2013/11/29 18:30:07, cpu wrote:
> don't we have a constant for "ntdll.dll"? if it is too cumbersome we can leave
> it as is.
> 
> Btw, I believe that (contrary to most of win32) the loader functions that take
> strings are natively single byte strings (ansi?) so calling GetModuleHandleA
is
> better.

There's a wide version of kNtdllName in internal_types.h, no ansi one. How about
I leave the string here and call GetModuleA?

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver.h (right):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
sandbox/win/src/service_resolver.h:47: virtual void AllowLocalPatches();
On 2013/11/29 18:30:07, cpu wrote:
> Do we want a way to unset this? 

I first wrote this with a |bool allow| parameter, but I couldn't find a case
where we would want to unset it so I removed it.

> 
> I am correct in thinking we don't run into issues because this lives in the
elf
> dll vs trying to autopatch AND being the broker in chrome.exe ?

That's correct.

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver_unittest.cc (right):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
sandbox/win/src/service_resolver_unittest.cc:262: &used);
On 2013/11/29 18:30:07, cpu wrote:
> indenting

Done.

[cpu_(ooo_6.6-7.5), 2013-12-01 00:18:44]

lgtm on my side

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver.cc (right):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
sandbox/win/src/service_resolver.cc:42: ntdll_base_ =
::GetModuleHandle(L"ntdll.dll");
On 2013/11/29 19:29:08, robertshield wrote:
> On 2013/11/29 18:30:07, cpu wrote:
> > don't we have a constant for "ntdll.dll"? if it is too cumbersome we can
leave
> > it as is.
> > 
> > Btw, I believe that (contrary to most of win32) the loader functions that
take
> > strings are natively single byte strings (ansi?) so calling GetModuleHandleA
> is
> > better.
> 
> There's a wide version of kNtdllName in internal_types.h, no ansi one. How
about
> I leave the string here and call GetModuleA?

Either fix sgtm.

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver.h (right):

https://codereview.chromium.org/84063004/diff/230001/sandbox/win/src/service_...
sandbox/win/src/service_resolver.h:47: virtual void AllowLocalPatches();
On 2013/11/29 19:29:08, robertshield wrote:
> On 2013/11/29 18:30:07, cpu wrote:
> > Do we want a way to unset this? 
> 
> I first wrote this with a |bool allow| parameter, but I couldn't find a case
> where we would want to unset it so I removed it.
> 

Yeah, I thought ntdll_base_ was a static global, being a member var means we
don't have the problem I was thinking which was on tests. So never mind.


> > 
> > I am correct in thinking we don't run into issues because this lives in the
> elf
> > dll vs trying to autopatch AND being the broker in chrome.exe ?
> 
> That's correct.

[rvargas (doing something else), 2013-12-02 20:05:33]

LGTM with minor nits.

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/nt_inter...
File sandbox/win/src/nt_internals.h (right):

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/nt_inter...
sandbox/win/src/nt_internals.h:605: IN void *dest,
nit: * should be part of the type, not the variable

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_globals.cc (right):

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_globals.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
nit: new style: no (c) (I believe) and 2013 :)

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_globals.cc:16: SANDBOX_INTERCEPT NtExports g_nt = { NULL
};
nit: use {}

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver.cc (right):

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/service_...
sandbox/win/src/service_resolver.cc:42: ntdll_base_ =
::GetModuleHandleA("ntdll.dll");
can we go the other way? I prefer reusing the same constant and have smaller
code than to have two constants and a path that is slightly faster.

[robertshield, 2013-12-03 18:53:50]

Thanks!

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/nt_inter...
File sandbox/win/src/nt_internals.h (right):

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/nt_inter...
sandbox/win/src/nt_internals.h:605: IN void *dest,
On 2013/12/02 20:05:33, rvargas wrote:
> nit: * should be part of the type, not the variable

Done.

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_globals.cc (right):

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_globals.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
On 2013/12/02 20:05:33, rvargas wrote:
> nit: new style: no (c) (I believe) and 2013 :)

Done.

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_globals.cc:16: SANDBOX_INTERCEPT NtExports g_nt = { NULL
};
On 2013/12/02 20:05:33, rvargas wrote:
> nit: use {}

Done.

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/service_...
File sandbox/win/src/service_resolver.cc (right):

https://codereview.chromium.org/84063004/diff/270001/sandbox/win/src/service_...
sandbox/win/src/service_resolver.cc:42: ntdll_base_ =
::GetModuleHandleA("ntdll.dll");
On 2013/12/02 20:05:33, rvargas wrote:
> can we go the other way? I prefer reusing the same constant and have smaller
> code than to have two constants and a path that is slightly faster.

Done.

[commit-bot: I haz the power, 2013-12-03 21:24:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/84063004/310001

[commit-bot: I haz the power, 2013-12-03 23:39:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/84063004/310001

[commit-bot: I haz the power, 2013-12-04 03:12:22]

Change committed as 238538