Plumb executable flag through proxy to base::SharedMemory::Create.

Plumb executable flag through proxy to base::SharedMemory::Create.

base::SharedMemory now takes a flag for whether executability is required.
Plumb that through the Linux-only proxy for this interface.

BUG= http://code.google.com/p/chromium/issues/detail?id=103377
TEST= nacl still works

R=mseaborn@chromium.org,jam@chromium.org

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

[Mark Seaborn, 2011-12-02 20:14:27]

http://codereview.chromium.org/8776053/diff/1/content/public/common/child_pro...
File content/public/common/child_process_sandbox_support_linux.h (right):

http://codereview.chromium.org/8776053/diff/1/content/public/common/child_pro...
content/public/common/child_process_sandbox_support_linux.h:18: // with
PROT_EXEC after making a mapping, but not that we'll need
The wording is a little unclear.  How about:

If 'executable' is set, this will return an object that can be mprotect()'d with
PROT_EXEC.  However, mmap()'ing the segment with PROT_EXEC will not necessarily
work because of peculiarities of how Linux implements the 'noexec' mount flag. 
[The second sentence is a guess from our discussions about the kernel
workarounds.]

[jam, 2011-12-02 20:42:22]

rubberstamp lgtm, deferring to Mark

[Roland McGrath, 2011-12-02 22:42:10]

I find your wording vastly less comprehensible than what I had before.
I've added a further explanation to the comment.  Please don't hold up the
review for such a petty matter.

[Mark Seaborn, 2011-12-02 23:07:18]

On 2 December 2011 14:42, <mcgrathr@chromium.org> wrote:

> I find your wording vastly less comprehensible than what I had before.
> I've added a further explanation to the comment.  Please don't hold up the
> review for such a petty matter.
>

I thought I replied relatively quickly.

"""
// The executable flag indicates that we'll need to use mprotect
// with PROT_EXEC after making a mapping, but not that we'll need
// to mmap with PROT_EXEC in the first place.
"""

'We' seems to refer to what MakeSharedMemorySegmentViaIPC() needs to do,
which isn't the case.  The 'executable' arg doesn't say what the caller
needs to do, just what it can do.

Mark

[Roland McGrath, 2011-12-02 23:37:06]

On 2011/12/02 23:07:18, Mark Seaborn wrote:
> I thought I replied relatively quickly.

What I mean is that you are preventing me from getting the job done while
you bikeshed a comment.  If I hadn't bothered to add the comment, I suspect
you would have given an LGTM in the first place.  

It's quite reasonable to say that a comment is unclear, but every other
reviewer I've seen says "LGTM, but please clarify this comment" in such
cases.  Since I'm following procedure, I'm not going to commit the code
until you say LGTM.  Hence, you are holding me up for this petty matter.
That is quite frustrating and likely to make me want to avoid you as a
reviewer in the future, which is not a good result for anyone.

> """
> // The executable flag indicates that we'll need to use mprotect
> // with PROT_EXEC after making a mapping, but not that we'll need
> // to mmap with PROT_EXEC in the first place.
> """
> 
> 'We' seems to refer to what MakeSharedMemorySegmentViaIPC() needs to do,
> which isn't the case.  The 'executable' arg doesn't say what the caller
> needs to do, just what it can do.

"We" refers to the caller's requirements on the file descriptor returned.
Since that was unclear to you, I am convinced that the comment would be
clearer by avoiding the use of such pronouns.  I've reworded it in that way.

PTAL


Thanks,
Roland

[Mark Seaborn, 2011-12-02 23:38:56]

LGTM