1447867 - Use shm_open for IPC shared memory on POSIX platforms

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P1)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

On almost all Unix-like systems, we can use shm_open and shm_unlink in base::SharedMemory instead of opening and unlinking temporary files.  This will allow us to delete (almost) all 1,200 lines of file_util from ipc/chromium.

On Linux with glibc, shm_open/shm_unlink is implemented by opening and unlinking temporary files in /dev/shm (i.e., we're duplicating code that the OS already has).  On other OSes it may use other, possibly more efficient, implementations; for example, this is the case on FreeBSD.

Android, however, does not support it.  But it has ashmem, which does what we want, and we already have code for it in the ipc/glue SharedMemoryBasic layer.  That could be merged into base::SharedMemory, which SharedMemoryBasic_android partially duplicates as it stands.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1447867 - Replace base::SharedMemory POSIX backend with shm_open and ashmem.

This replaces using file_util to open and unlink temporary files
(/dev/shm on Linux, $TMPDIR or /tmp otherwise) with the POSIX shm_open
API, or ashmem on Android (which doesn't implement shm_open).

glibc maps shm_open/shm_unlink to open and unlink in /dev/shm (as does
musl libc), so the Linux situation is mostly unchanged except we aren't
duplicating code from system libraries.  Other OSes may (and some do)
use more efficient implementations than temporary files.

FreeBSD's SHM_ANON extension is used if available.  Sadly, it's not
standard; it would make this patch much simpler if it were.

This patch changes the shm file names; they now start with "org.mozilla"
instead of "org.chromium" because the original Chromium code is mostly
gone at this point.  When running as a Snap package, the required
filename prefix is added; other container/sandbox environments using
AppArmor to restrict the allowed filenames may need to be adjusted.

The shm names now include the creating process's pid, to allow
using sandboxing to prevent interfering with shm belonging to other
applications or other processes within the same browser instance.

Review commit: https://reviewboard.mozilla.org/r/237454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237454/

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8968759 [details]
Bug 1447867 - Replace base::SharedMemory POSIX backend with shm_open and ashmem.

https://reviewboard.mozilla.org/r/237454/#review243362

r=me

::: ipc/chromium/src/base/shared_memory_posix.cc:117
(Diff revision 1)
> +      if (shm_unlink(name.c_str()) != 0) {
> +        DLOG(FATAL) << "failed to unlink shm: " << strerror(errno);

Just to be clear here, if `shm_unlink` fails, we will treat the overall operation as success, and return the `shm_open`'d thing?  Because the fd will still be `>= 0`.

I guess it's not worth trying again with a different name, because `shm_unlink` will probably fail on that name, too?  But maybe we should `close` and return failure?  Except that the named shared memory segment is still there?  Not sure what the right way forward is.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8968759 [details]
> > +      if (shm_unlink(name.c_str()) != 0) {
> > +        DLOG(FATAL) << "failed to unlink shm: " << strerror(errno);
> 
> Just to be clear here, if `shm_unlink` fails, we will treat the overall
> operation as success, and return the `shm_open`'d thing?  Because the fd
> will still be `>= 0`.
> 
> I guess it's not worth trying again with a different name, because
> `shm_unlink` will probably fail on that name, too?  But maybe we should
> `close` and return failure?  Except that the named shared memory segment is
> still there?  Not sure what the right way forward is.

Yes, there really isn't a “good” way to deal with this.  But on second thought, the close-and-fail option seems less bad: if we're going to leak a segment, probably better to leak a zero-length segment than a possibly large one; if it somehow *was* unlinked it's an unnecessary failure, but that doesn't seem likely.  As for retrying with a different name, I suppose that might work if the cause is transient, but it might also make us leak files until /dev/shm runs out of inodes (or equivalent).

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8968759 [details]
Bug 1447867 - Replace base::SharedMemory POSIX backend with shm_open and ashmem.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237454/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a024d5ca77b
Replace base::SharedMemory POSIX backend with shm_open and ashmem. r=froydnj

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1a024d5ca77b

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Backed out: https://hg.mozilla.org/mozilla-central/rev/7793bcae69f6

Comment 8

[Kris Maglione [:kmag]]

It might be worth trying to re-land this now that bug 1475612 is fixed. Pretty good odds it's the same issue.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Kris Maglione [:kmag] from comment #8)
> It might be worth trying to re-land this now that bug 1475612 is fixed.
> Pretty good odds it's the same issue.

I'm not so sure.  This patch doesn't change anything the child does; it just changes how the fd is created in the parent, and a few things about the fd that normally wouldn't matter (close-on-exec set, append not set, offset at 0 instead of end).  Also, there doesn't seem to be a dup() for the changed prefs like there is with the shared prefs, so nothing else should think it ever had that fd.  And the bug 1455828 crashes were from reading incorrect data (and not NUL bytes either), not EBADF.

I'm going to reland it anyway (after bug 1243108, so we don't start crashing again from spurious unlink failures), because I suspect the cause was bug 1456911.

My other guess was that something was somehow writing to the wrong fd in the child, and this patch changing the append flag and file offset made it overwrite the pref blob instead of writing harmlessly after the end, but I don't have an explanation for why something would just write to an fd that hadn't been available at any point since the process was started.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #9)
> (In reply to Kris Maglione [:kmag] from comment #8)
> > It might be worth trying to re-land this now that bug 1475612 is fixed.
> > Pretty good odds it's the same issue.
> 
> I'm not so sure.  This patch doesn't change anything the child does; it just
> changes how the fd is created in the parent, and a few things about the fd
> that normally wouldn't matter (close-on-exec set, append not set, offset at
> 0 instead of end).  Also, there doesn't seem to be a dup() for the changed
> prefs like there is with the shared prefs, so nothing else should think it
> ever had that fd.  And the bug 1455828 crashes were from reading incorrect
> data (and not NUL bytes either), not EBADF.

There doesn't need to be a dup(), just an fd that can be opened and then replaced with an fd from another open operation.

Some of the crashes from bug 1475612 were EBADF failures, but a lot were actually in the content process, when we wound up trying to decode data that we expected to be a shared string map, but wound up being some arbitrary file instead.

All that's necessary for the double close to start causing that issue is a slight change in timing. Most of the crashes in the child process actually only started showing up after an unrelated patch changed some timings enough for the fd to be closed at just the right time for that to happen.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83bab8cf29bf
Replace base::SharedMemory POSIX backend with shm_open and ashmem. r=froydnj

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/83bab8cf29bf