Give base::SharedMemory::CreateAnonymous an executable flag

Give base::SharedMemory::CreateAnonymous an executable flag

NaCl on Mac and Linux needs to create a shared memory object that it can
later make executable with mprotect.  Express this need in the interface it
uses.  Add a test that pages mapped from such an object can later be passed
to mprotect with PROT_EXEC.

This lays the groundwork for a later change that will sometimes use a
different method to allocate an object on Linux when it needs to be
executable.  On some Linux distributions, shm_open yields objects whose
mappings cannot be made executable.

BUG= http://code.google.com/p/chromium/issues/detail?id=103377
TEST= SharedMemory.AnonymousExecutable

R=mark@chromium.org,jam@chromium.org,amit@chromium.org,ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112570

[Roland McGrath, 2011-11-16 21:40:46]

I listed so many reviewers just trying to cover all the OWNERS I found:

base: mark@chromium.org
content: jam@chromium.org
chrome_frame: amit@chromium.org
ui: ben@chromium.org

The real change is just in base/ for Mark.
The rest are just trivial adaptations to the method signature change.

PTAL


Thanks,
Roland

[Mark Seaborn, 2011-11-16 22:34:01]

I discussed this with Roland.  I think we want to put the logic into NaCl's
CreateMemoryObject() function instead.

Mark

[jam, 2011-11-17 01:22:07]

content lgtm

[Roland McGrath, 2011-11-18 23:19:13]

It is looking like AppArmor may preclude the alternate tack we were considering.
 It's not settled yet.  But we might need to go this route after all.  So all
OWNERS please take a look and sign off on this on its own merits in case we
really do need this in.

[Roland McGrath, 2011-11-29 19:44:27]

Ping OWNERS...mark, amit, ben

[Mark Mentovai, 2011-11-29 19:46:50]

You asked us to look “in case we really do need this in.” I can generally do a
review pretty quickly if we decide that we really do need it, but it would be
wasteful to review this if we don’t need it. What does the decision about
whether or not to use this hinge on?

[Roland McGrath, 2011-11-29 20:05:23]

On 2011/11/29 19:46:50, Mark Mentovai wrote:
> You asked us to look “in case we really do need this in.” I can generally do a
> review pretty quickly if we decide that we really do need it, but it would be
> wasteful to review this if we don’t need it. What does the decision about
> whether or not to use this hinge on?

I've decided that we do in fact need to use it.  I tried taking another
tack and found that there were unanticipated issues preventing that
solution.

[Mark Mentovai, 2011-11-29 20:08:28]

OK. Then I have one high-level comment. As you extend functions like this, it
becomes cumbersome to maintain all of the call sites, and trying to remember
which bool does what makes the code harder to read. Have you considered using an
“options” struct as was done for base::LaunchProcess (base/process_util.h),
which accepts a LaunchOptions struct?

[Mark Mentovai, 2011-11-29 20:10:43]

http://codereview.chromium.org/8585002/diff/1/base/metrics/stats_table.cc
File base/metrics/stats_table.cc (right):

http://codereview.chromium.org/8585002/diff/1/base/metrics/stats_table.cc#new...
base/metrics/stats_table.cc:171: if (!priv->shared_memory_.CreateNamed(name,
true, size, false))
I’m looking at uses like this. What’s true? What’s false? Gotta go read the
header for shared memory. That’s too bad.

As an alternative to the options struct, you can replace the bools with named
enums, so callers can pass values like OPEN_EXISTING. But that still leaves the
problem that extending the function requires you to touch all of the call sites,
which isn’t the case with an options struct.

[Roland McGrath, 2011-11-29 21:03:16]

On 2011/11/29 20:08:28, Mark Mentovai wrote:
> OK. Then I have one high-level comment. As you extend functions like this, it
> becomes cumbersome to maintain all of the call sites, and trying to remember
> which bool does what makes the code harder to read. Have you considered using
an
> “options” struct as was done for base::LaunchProcess (base/process_util.h),
> which accepts a LaunchOptions struct?

I was going for minimal change to the existing interface.
I have no particular opinions about the exact interface choices.
If you'd like me to convert to an options struct, I can do that.

Conversely, the really minimal change would be to leave the existing
interfaces as they are and just add a new CreateAnonymousExecutable
method.

I'll do whatever you prefer.


Thanks,
Roland

[Mark Mentovai, 2011-11-29 21:09:13]

I think having two bools is bad interface design.

My preference would be to put the bools (and maybe size?) into an options
struct. Second preference is to do the enum thing I suggested. Adding a new
function call for the execute bit is a distant third.

[Roland McGrath, 2011-11-29 23:55:23]

I made a struct for all the creation parameters and a generic Create() method
taking that.

I left the existing Create* calls as front-ends to Create(), so all the old call
sites can remain unchanged.  (So if Mark likes something like this version, no
other OWNERS need to sign off.)

I made those front-ends inlines in the interface header because they are trivial
and to avoid duplicating the identical code in the posix and win implementation
source files.

[Mark Mentovai, 2011-12-01 19:31:16]

I think this is workable as an interface now.

http://codereview.chromium.org/8585002/diff/9002/base/shared_memory.h
File base/shared_memory.h (right):

http://codereview.chromium.org/8585002/diff/9002/base/shared_memory.h#newcode45
base/shared_memory.h:45: {}
{} belongs on the preceding line, or { on the preceding line and } on this line.

Same on line 51.

http://codereview.chromium.org/8585002/diff/9002/base/shared_memory.h#newcode47
base/shared_memory.h:47: explicit SharedMemoryCreateOptions(uint32 isize) :
name(NULL),
Why do we need this form of the constructor? Can’t we just let callers set size
once they’ve established a local SharedMemoryCreateOptions object?

http://codereview.chromium.org/8585002/diff/9002/base/shared_memory_android.cc
File base/shared_memory_android.cc (right):

http://codereview.chromium.org/8585002/diff/9002/base/shared_memory_android.c...
base/shared_memory_android.cc:24: options.name.c_str(),
The formatting here makes it look kind of like there are three arguments. Maybe
indent this line by another 4. Maybe even put the |"" : | on this line too.

http://codereview.chromium.org/8585002/diff/9002/base/shared_memory_win.cc
File base/shared_memory_win.cc (right):

http://codereview.chromium.org/8585002/diff/9002/base/shared_memory_win.cc#ne...
base/shared_memory_win.cc:1: // Copyright (c) 2006-2011 The Chromium Authors.
All rights reserved.
Just 2011, no 2006.

http://codereview.chromium.org/8585002/diff/9002/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/8585002/diff/9002/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:495:
base::SharedMemoryCreateOptions options(1);  // size
Yeah, see, this |// size| is cumbersome. It’d be better to just set options.size
on the next line.

[Roland McGrath, 2011-12-01 21:17:29]

I think I've addressed all your comments.
PTAL


Thanks,
Roland

[Mark Mentovai, 2011-12-01 21:26:01]

LGTM

http://codereview.chromium.org/8585002/diff/17001/base/shared_memory.h
File base/shared_memory.h (right):

http://codereview.chromium.org/8585002/diff/17001/base/shared_memory.h#newcode42
base/shared_memory.h:42: struct SharedMemoryCreateOptions {
To promote more efficient memory use, please order the members in this struct
more appropriately:

pointer, uint32, bool, bool