Implement SharedMemory::NewAnonymousReadOnly(contents).

Implement SharedMemory::ShareReadOnlyToProcess().

This avoids potential security holes where the renderer could be exploited and
then write into space shared by other renderers or even the browser.

I've done this on Posix by opening both a read/write and read-only file descriptor to the same file. Then ShareReadOnlyToProcess dup()s the read-only descriptor instead of the read/write one. It's an error to try to ShareReadOnly from a SharedMemory that was created from a single SharedMemoryHandle.

The test checks that operations strictly through the file handle can't get
write access to the memory. On Linux there's still a hole through /dev/fd
in the filesystem, but jln@ assures me that the sandbox prevents the
filesystem-based attack. We should eventually write an explicit test for this.

Android needs http://crbug.com/320865 figured out.

BUG=302724, 320865
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236347

[Jeffrey Yasskin, 2013-10-15 23:03:52]

PTAL.

Albert, are you comfortable LGTM'ing once the security folks are happy, or do
you want me to find another base/ owner?

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:110: base::ThreadRestrictions::ScopedAllowIO
allow_io;
This causes a presubmit error:

Banned functions were used.
    base/memory/shared_memory_posix.cc:110:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.


Let me know if there's something else I should do instead.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
File base/memory/shared_memory_unittest.cc (right):

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_unittest.cc:433: #elif OS_WIN
Please try to think of other attacks I should be testing on Windows. I don't
know the API there very well.

[jln (very slow on Chromium), 2013-10-16 00:16:38]

Thanks a lot for taking on this Jeffrey!

Will or Justin: could you do another pass over the Windows-specific code?

This looks good in general. I'm only worried about the existing API giving a
false sense of security with the existing read_only flag.

See my suggestion in the interface. At the very least a comment is needed. If
there is significant broken usage, feel free to just add a TODO(jln, wfh) to
clean this up.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
File base/memory/shared_memory.h (right):

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory.h:83: // shared memory file.
Please, add a comment explaining that this is not safe and doesn't do what it's
supposed to do.

Even better:
- Can it be removed?
- Can it be DCHECK-ed for the case where "read_only" would require a permission
change?

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
File base/memory/shared_memory_posix.cc (right):

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_posix.cc:110: base::ThreadRestrictions::ScopedAllowIO
allow_io;
On 2013/10/15 23:03:52, Jeffrey Yasskin wrote:
> This causes a presubmit error:
> 
> Banned functions were used.
>     base/memory/shared_memory_posix.cc:110:
>       New code should not use ScopedAllowIO. Post a task to the blocking
>       pool or the FILE thread instead.
> 
> 
> Let me know if there's something else I should do instead.

I would hope that the presubmit warning doesn't concern base/

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_posix.cc:117: return scoped_ptr<SharedMemory>();
Should we CHECK here instead? (Or LOG(FATAL)). It looks error-prone.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_posix.cc:122: LOG(ERROR) << "Failed to reopen temp
shmem file readonly.";
Use PLOG instead.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_posix.cc:123: return scoped_ptr<SharedMemory>();
Same remark.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_posix.cc:128: PLOG(WARNING) << "unlink";
I would personally add a DCHECK. Your choice.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_posix.cc:134: return scoped_ptr<SharedMemory>();
Same remark about error handling.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_posix.cc:142: fchmod(readonly_fd, S_IRUSR);
Please, at least DCHECK this. It's too easy to silently have no-ops.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
File base/memory/shared_memory_unittest.cc (right):

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_unittest.cc:385: #if OS_POSIX
Style:
#if defined()

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_unittest.cc:421: fchmod(handle.fd, S_IRUSR | S_IWUSR);
Please, check the return value, it's better to keep the test tight.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_unittest.cc:426: // Mac appears to get this right by
treating open(/dev/fd/N) as dup(N).
This is not correct, open(/dev/fd/N) is very different from dup(). dup()
duplicates a reference to a file description, open(/dev/fd/N) will create a new
file description (for instance advisory locks and seeks are shared when duping).

OS X get this right by enforcing the requested access mode to be a subset of the
existing access mode.

Could you explicitly call out a problem in Linux (you can still say "OS X get
this right). It makes the following !OS_LINUX more natural.

https://chromiumcodereview.appspot.com/27265002/diff/29001/base/memory/shared...
base/memory/shared_memory_unittest.cc:427: #if !OS_LINUX
Style: #if !defined()

[Jeffrey Yasskin, 2013-10-16 01:16:39]

Let's see how the trybots feel about the stricter checks. :)

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory.h
File base/memory/shared_memory.h (right):

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory.h:83: // shared memory file.
On 2013/10/16 00:16:39, jln wrote:
> Please, add a comment explaining that this is not safe and doesn't do what
it's
> supposed to do.
> 
> Even better:
> - Can it be removed?
> - Can it be DCHECK-ed for the case where "read_only" would require a
permission
> change?

I've added the comment. I'd want to make the behavioral change in a separate CL,
so I added a TODO.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:117: return scoped_ptr<SharedMemory>();
On 2013/10/16 00:16:39, jln wrote:
> Should we CHECK here instead? (Or LOG(FATAL)). It looks error-prone.

Create() only LOG(FATAL)s when /dev/shm is unwritable, so I'm reluctant to make
this API crash in more cases. If callers use the result without checking,
they'll get a NULL dereference anyway, which is better than Create().

If we want to ratchet up the crashes here (which is plausible), I think we
should do it in a sequence of subsequent CLs, to reduce the risk of this one.

If you really think this needs to be done in this CL for one or more of these
NULL returns, let me know.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:122: LOG(ERROR) << "Failed to reopen temp
shmem file readonly.";
On 2013/10/16 00:16:39, jln wrote:
> Use PLOG instead.

Oh yeah, done.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:128: PLOG(WARNING) << "unlink";
On 2013/10/16 00:16:39, jln wrote:
> I would personally add a DCHECK. Your choice.

I'm imitating Create(), so I'd leave this for a subsequent CL also.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:137: writable_fp.reset();
I've also changed this to check the fclose() result and return NULL if it fails.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:142: fchmod(readonly_fd, S_IRUSR);
On 2013/10/16 00:16:39, jln wrote:
> Please, at least DCHECK this. It's too easy to silently have no-ops.

Done.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
File base/memory/shared_memory_unittest.cc (right):

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_unittest.cc:385: #if OS_POSIX
On 2013/10/16 00:16:39, jln wrote:
> Style:
> #if defined()

Done.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_unittest.cc:421: fchmod(handle.fd, S_IRUSR | S_IWUSR);
On 2013/10/16 00:16:39, jln wrote:
> Please, check the return value, it's better to keep the test tight.

Done.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_unittest.cc:426: // Mac appears to get this right by
treating open(/dev/fd/N) as dup(N).
On 2013/10/16 00:16:39, jln wrote:
> This is not correct, open(/dev/fd/N) is very different from dup(). dup()
> duplicates a reference to a file description, open(/dev/fd/N) will create a
new
> file description (for instance advisory locks and seeks are shared when
duping).

https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag...
actually says that:

the call:

           fd = open("/dev/fd/0", mode);

     and the call:

           fd = fcntl(0, F_DUPFD, 0);

     are equivalent.

Which makes me think advisory locks and seeks would be shared. But I'm happy to
avoid claiming that in the comment.

> OS X get this right by enforcing the requested access mode to be a subset of
the
> existing access mode.
> 
> Could you explicitly call out a problem in Linux (you can still say "OS X get
> this right). It makes the following !OS_LINUX more natural.

Done.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_unittest.cc:427: #if !OS_LINUX
On 2013/10/16 00:16:39, jln wrote:
> Style: #if !defined()

Done.

[jschuh, 2013-10-16 13:32:32]

This interface feels awkward. Wouldn't it be better to add a
ShareReadOnlyToProcess and GiveReadOnlyToProcess? Or, add a read-only parameter
to the existing ShareToProcess and GiveToProcess method (accepting that a bunch
of all callers would need to be cleaned up)?

[Will Harris, 2013-10-16 17:01:02]

https://codereview.chromium.org/27265002/diff/66001/base/memory/shared_memory...
File base/memory/shared_memory_unittest.cc (right):

https://codereview.chromium.org/27265002/diff/66001/base/memory/shared_memory...
base/memory/shared_memory_unittest.cc:456:
::DuplicateHandle(GetCurrentProcess(),
should probably CloseHandle after this succeeds, or just use
base::win::ScopedHandle

[jln (very slow on Chromium), 2013-10-16 17:51:14]

I have to rush to a meeting, I'll take another quick look later, but this seems
good.

Let's discuss though if it would be possible to just amend the existing
interface, as per Justin comment.

I think we didn't start in that direction because it wasn't clear to us at the
time that OS X did support /dev/fd/N.

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
File base/memory/shared_memory_unittest.cc (right):

https://codereview.chromium.org/27265002/diff/29001/base/memory/shared_memory...
base/memory/shared_memory_unittest.cc:426: // Mac appears to get this right by
treating open(/dev/fd/N) as dup(N).
>            fd = fcntl(0, F_DUPFD, 0);

Ohh yeah, F_DUPFD is confusing. It really has nothing to do with dup(). dup()
merely gives you a new descriptor (a reference to a file description), while
F_DUPFD gives you a copy of a file description (still pointing to the same inode
though).

i.e. remember that there are two levels of dereference between a fd and an
inode.

[Jeffrey Yasskin, 2013-10-16 22:27:52]

Here's a version that defines ShareReadOnlyToProcess and GiveReadOnlyToProcess.
After looking through some of the ShareToProcess callers, I think we'll need to
change a lot of them to use these read-only versions. It's not just extension
content scripts.

https://codereview.chromium.org/27265002/diff/66001/base/memory/shared_memory...
File base/memory/shared_memory_unittest.cc (right):

https://codereview.chromium.org/27265002/diff/66001/base/memory/shared_memory...
base/memory/shared_memory_unittest.cc:456:
::DuplicateHandle(GetCurrentProcess(),
On 2013/10/16 17:01:03, Will Harris wrote:
> should probably CloseHandle after this succeeds, or just use
> base::win::ScopedHandle

Thanks, done with ScopedHandle.

[Jeffrey Yasskin, 2013-10-16 23:22:21]

https://codereview.chromium.org/27265002/diff/89001/base/memory/shared_memory...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/27265002/diff/89001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:417: StringPrintf("/dev/fd/%d",
mapped_file_);
Note that this is going to fail on Android, which doesn't have /dev/fd. It might
have /proc/self/fd, so I've kicked off https://codereview.chromium.org/26330008
to check. If that doesn't exist either, do you folks have any suggestions?

[jln (very slow on Chromium), 2013-10-16 23:58:05]

https://codereview.chromium.org/27265002/diff/89001/base/memory/shared_memory...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/27265002/diff/89001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:417: StringPrintf("/dev/fd/%d",
mapped_file_);
On 2013/10/16 23:22:21, Jeffrey Yasskin wrote:
> Note that this is going to fail on Android, which doesn't have /dev/fd. It
might
> have /proc/self/fd, so I've kicked off
https://codereview.chromium.org/26330008
> to check. If that doesn't exist either, do you folks have any suggestions?

Android definitely does have /proc/self/fd, no problem.

[Jeffrey Yasskin, 2013-10-17 03:00:25]

https://codereview.chromium.org/27265002/diff/99001/base/memory/shared_memory...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/27265002/diff/99001/base/memory/shared_memory...
base/memory/shared_memory_posix.cc:423: new_fd =
HANDLE_EINTR(open(writable_fd_path.c_str(), O_RDONLY));
This approach doesn't work on Apple OSen. 
iOS:
http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...
MacOS:
http://build.chromium.org/p/tryserver.chromium/builders/swarm_triggered/build...


[ RUN      ] SharedMemoryTest.ShareReadOnly
/b/build/slave/ios_dbg_simulator/build/src/base/memory/shared_memory_unittest.cc:397:
Failure
Value of: fcntl(handle.fd, 3) & 0x0003
  Actual: 2
Expected: 0x0000
Which is: 0
The descriptor itself should be read-only.
/b/build/slave/ios_dbg_simulator/build/src/base/memory/shared_memory_unittest.cc:408:
Failure
Value of: writable
  Actual: 0x9fee000
Expected: ((void *)-1)
Which is: 0xffffffff
It shouldn't be possible to re-mmap the descriptor writable.


To support Apple, it looks like we need to create the read-only descriptor at
the same time we create the writable one. What API would you like?

[jln (very slow on Chromium), 2013-10-17 19:15:10]

On 2013/10/17 03:00:25, Jeffrey Yasskin wrote:
>
https://codereview.chromium.org/27265002/diff/99001/base/memory/shared_memory...
> File base/memory/shared_memory_posix.cc (right):
> 
>
https://codereview.chromium.org/27265002/diff/99001/base/memory/shared_memory...
> base/memory/shared_memory_posix.cc:423: new_fd =
> HANDLE_EINTR(open(writable_fd_path.c_str(), O_RDONLY));
> This approach doesn't work on Apple OSen. 
> iOS:
>
http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...
> MacOS:
>
http://build.chromium.org/p/tryserver.chromium/builders/swarm_triggered/build...
> 
> 
> [ RUN      ] SharedMemoryTest.ShareReadOnly
>
/b/build/slave/ios_dbg_simulator/build/src/base/memory/shared_memory_unittest.cc:397:
> Failure
> Value of: fcntl(handle.fd, 3) & 0x0003
>   Actual: 2
> Expected: 0x0000
> Which is: 0
> The descriptor itself should be read-only.
>
/b/build/slave/ios_dbg_simulator/build/src/base/memory/shared_memory_unittest.cc:408:
> Failure
> Value of: writable
>   Actual: 0x9fee000
> Expected: ((void *)-1)
> Which is: 0xffffffff
> It shouldn't be possible to re-mmap the descriptor writable.
> 
> 
> To support Apple, it looks like we need to create the read-only descriptor at
> the same time we create the writable one. What API would you like?

Arg, OS X hates freedom.
Robert, Ian, Mark: is there any way to re-open an unlinked inode with a more
restricted access mode on OS X? open(/dev/fd/X, O_RDONLY) doesn't work, it
silently re-opens it RW. Apparently OSX guarantees that an open of /dev/fd/X is
equivalent to fcntl(F_DUPFD..), regardless of the specified access mode.

Jeffrey: I'm almost wondering if we should not just open() twice and keep both
file descriptor around. Mark may have some other API suggestion as well as a
base owner.

[Jeffrey Yasskin, 2013-10-21 16:28:25]

On 2013/10/17 19:15:10, jln wrote:
> On 2013/10/17 03:00:25, Jeffrey Yasskin wrote:
> >
>
https://codereview.chromium.org/27265002/diff/99001/base/memory/shared_memory...
> > File base/memory/shared_memory_posix.cc (right):
> > 
> >
>
https://codereview.chromium.org/27265002/diff/99001/base/memory/shared_memory...
> > base/memory/shared_memory_posix.cc:423: new_fd =
> > HANDLE_EINTR(open(writable_fd_path.c_str(), O_RDONLY));
> > This approach doesn't work on Apple OSen. 
> > iOS:
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...
> > MacOS:
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/swarm_triggered/build...
> > 
> > 
> > [ RUN      ] SharedMemoryTest.ShareReadOnly
> >
>
/b/build/slave/ios_dbg_simulator/build/src/base/memory/shared_memory_unittest.cc:397:
> > Failure
> > Value of: fcntl(handle.fd, 3) & 0x0003
> >   Actual: 2
> > Expected: 0x0000
> > Which is: 0
> > The descriptor itself should be read-only.
> >
>
/b/build/slave/ios_dbg_simulator/build/src/base/memory/shared_memory_unittest.cc:408:
> > Failure
> > Value of: writable
> >   Actual: 0x9fee000
> > Expected: ((void *)-1)
> > Which is: 0xffffffff
> > It shouldn't be possible to re-mmap the descriptor writable.
> > 
> > 
> > To support Apple, it looks like we need to create the read-only descriptor
at
> > the same time we create the writable one. What API would you like?
> 
> Arg, OS X hates freedom.
> Robert, Ian, Mark: is there any way to re-open an unlinked inode with a more
> restricted access mode on OS X? open(/dev/fd/X, O_RDONLY) doesn't work, it
> silently re-opens it RW. Apparently OSX guarantees that an open of /dev/fd/X
is
> equivalent to fcntl(F_DUPFD..), regardless of the specified access mode.
> 
> Jeffrey: I'm almost wondering if we should not just open() twice and keep both
> file descriptor around. Mark may have some other API suggestion as well as a
> base owner.

Anyone have suggestions? I can start on the "open it twice" approach, but I'll
have to put in some CHECKs for when the descriptor has been shared writable
(which will drop the read-only descriptor), and then the user tries to share it
read-only.

I'd like some assurance that the reviewers will approve an implementation that
follows Julien's suggestion.

[Robert Sesek, 2013-10-21 20:10:24]

On 2013/10/21 16:28:25, Jeffrey Yasskin wrote:
> On 2013/10/17 19:15:10, jln wrote:
> > On 2013/10/17 03:00:25, Jeffrey Yasskin wrote:
> > >
> >
>
https://codereview.chromium.org/27265002/diff/99001/base/memory/shared_memory...
> > > File base/memory/shared_memory_posix.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/27265002/diff/99001/base/memory/shared_memory...
> > > base/memory/shared_memory_posix.cc:423: new_fd =
> > > HANDLE_EINTR(open(writable_fd_path.c_str(), O_RDONLY));
> > > This approach doesn't work on Apple OSen. 
> > > iOS:
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...
> > > MacOS:
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/swarm_triggered/build...
> > > 
> > > 
> > > [ RUN      ] SharedMemoryTest.ShareReadOnly
> > >
> >
>
/b/build/slave/ios_dbg_simulator/build/src/base/memory/shared_memory_unittest.cc:397:
> > > Failure
> > > Value of: fcntl(handle.fd, 3) & 0x0003
> > >   Actual: 2
> > > Expected: 0x0000
> > > Which is: 0
> > > The descriptor itself should be read-only.
> > >
> >
>
/b/build/slave/ios_dbg_simulator/build/src/base/memory/shared_memory_unittest.cc:408:
> > > Failure
> > > Value of: writable
> > >   Actual: 0x9fee000
> > > Expected: ((void *)-1)
> > > Which is: 0xffffffff
> > > It shouldn't be possible to re-mmap the descriptor writable.
> > > 
> > > 
> > > To support Apple, it looks like we need to create the read-only descriptor
> at
> > > the same time we create the writable one. What API would you like?
> > 
> > Arg, OS X hates freedom.
> > Robert, Ian, Mark: is there any way to re-open an unlinked inode with a more
> > restricted access mode on OS X? open(/dev/fd/X, O_RDONLY) doesn't work, it
> > silently re-opens it RW. Apparently OSX guarantees that an open of /dev/fd/X
> is
> > equivalent to fcntl(F_DUPFD..), regardless of the specified access mode.
> > 
> > Jeffrey: I'm almost wondering if we should not just open() twice and keep
both
> > file descriptor around. Mark may have some other API suggestion as well as a
> > base owner.
> 
> Anyone have suggestions? I can start on the "open it twice" approach, but I'll
> have to put in some CHECKs for when the descriptor has been shared writable
> (which will drop the read-only descriptor), and then the user tries to share
it
> read-only.
> 
> I'd like some assurance that the reviewers will approve an implementation that
> follows Julien's suggestion.

Sorry, I wanted to track this down in the OS X kernel sources. In
http://www.opensource.apple.com/source/xnu/xnu-2050.24.15/bsd/miscfs/devfs/de...,
which sets up the /dev/fd filesystem, you can see at the bottom that they map
vnop_open_desc to fdesc_open(). As you suspect, this merely sets the existing fd
as an fd to be dup'd. This bubbles up to open1() in bsd/vfs/vfs_syscalls.c,
which then calls dupfdopen() to actually perform the dup. They do acknowledge
that it's a kludge at least ;). I can't think of a way to do what you want using
POSIX APIs. Mach shared memory can do this, but Chrome is not set up to use
that. I think your best route is to create the read-only fd at the time of shmem
creation, but I'm not a base/OWNERS so I can't attest to that approach.

[Mark Mentovai, 2013-10-25 19:53:22]

I think that the best approach is to do a pair of opens, taking care to check
that you’ve actually opened the same thing both times (fstat checking st_dev and
st_ino?)

[Jeffrey Yasskin, 2013-11-19 00:03:06]

This is finally ready. I bailed on getting Android to work, for now:
http://crbug.com/320865.

[jschuh, 2013-11-19 00:34:26]

LGTM on the general API and Windows implementation.

[Mark Mentovai, 2013-11-20 14:21:41]

LGTM

https://codereview.chromium.org/27265002/diff/549001/base/memory/shared_memor...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/27265002/diff/549001/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:157: // closed,
Reflow this comment, this word doesn’t need to be so lonely.

https://codereview.chromium.org/27265002/diff/549001/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:276: return PrepareMapFile(fp.Pass(),
ScopedFD(&readonly_fd));
Move the ScopedFD up (like you did above) to better handle early returns. These
can both be .Pass().

https://codereview.chromium.org/27265002/diff/549001/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:338: if
(HANDLE_EINTR(close(readonly_mapped_file_)) < 0)
I have determined that it’s never correct to HANDLE_EINTR close.

This is always true on Linux. The story is sadder on Mac, but the bottom line is
that I did some heinous stuff to make it true there too. More details (if you’re
interested) at http://crbug.com/269623 .

I still haven’t done a cleanup of all of the HANDLE_EINTR(close) sites yet, but
new code should be written correctly, so you can fix this line, line 195, line
208, and line 333 while you’re here.

[Jeffrey Yasskin, 2013-11-20 18:02:09]

https://codereview.chromium.org/27265002/diff/549001/base/memory/shared_memor...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/27265002/diff/549001/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:157: // closed,
On 2013/11/20 14:21:41, Mark Mentovai wrote:
> Reflow this comment, this word doesn’t need to be so lonely.

Done.

https://codereview.chromium.org/27265002/diff/549001/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:276: return PrepareMapFile(fp.Pass(),
ScopedFD(&readonly_fd));
On 2013/11/20 14:21:41, Mark Mentovai wrote:
> Move the ScopedFD up (like you did above) to better handle early returns.
These
> can both be .Pass().

Done.

https://codereview.chromium.org/27265002/diff/549001/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:338: if
(HANDLE_EINTR(close(readonly_mapped_file_)) < 0)
On 2013/11/20 14:21:41, Mark Mentovai wrote:
> I have determined that it’s never correct to HANDLE_EINTR close.
> 
> This is always true on Linux. The story is sadder on Mac, but the bottom line
is
> that I did some heinous stuff to make it true there too. More details (if
you’re
> interested) at http://crbug.com/269623 .
> 
> I still haven’t done a cleanup of all of the HANDLE_EINTR(close) sites yet,
but
> new code should be written correctly, so you can fix this line, line 195, line
> 208, and line 333 while you’re here.

Oh, ok; done. It was very slightly easier to unwrap all the close()s in the
file, if that's all right.

[commit-bot: I haz the power, 2013-11-20 18:02:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/27265002/839001

[commit-bot: I haz the power, 2013-11-20 22:00:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/27265002/839001

[commit-bot: I haz the power, 2013-11-20 23:41:27]

Change committed as 236347