Cubeb audioipc requires a named Unix-domain socket

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jld, Assigned: kinetik)

Tracking

(Blocks 1 bug)

Trunk
mozilla59
Unspecified
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 disabled, firefox59 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

I noticed that the audioipc client connects to a named Unix-domain socket, which means we'll need to retain the workaround from bug 1259508 to pre-load cubeb before starting sandboxing on Linux.

Pre-acquiring resources like this isn't necessarily a bad thing — for example, the media plugin container on Linux pre-opens various files, including the plugin itself, so that the sandbox can completely remove filesystem access (vs. using a broker and increasing attack surface).

The alternative is to pass down one end of a socketpair over IPDL and then run the dedicated protocol over that.  This might be nontrivial to implement in this case, since the content process would be initiating the request for audio services.  The main benefit would be delaying audio initialization until when/if content requests it; I don't know whether this would have a significant effect on startup performance metrics.

In any case, this doesn't affect the strength of the sandbox, so from a sandboxing point of view I'd consider it lower priority than getting AudioIPC shipped and enabled by default.
Rank: 25
Priority: -- → P3
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #0)
> The alternative is to pass down one end of a socketpair over IPDL and then
> run the dedicated protocol over that.  This might be nontrivial to implement
> in this case, since the content process would be initiating the request for
> audio services.  The main benefit would be delaying audio initialization
> until when/if content requests it; I don't know whether this would have a
> significant effect on startup performance metrics.

Would it be possible to pass one end of a socketpair into the content process preemptively at process creation time, which the client can then use later when trying to create the libcubeb context?  It seems like it'd be much simpler to implement, but it does waste an fd in every content process that ends up not using audio.
Forgot to mention: I ask because I was trying to get audioipc running in Firefox on macOS, which should work more-or-less right now, but I'm hitting issues with the sandbox that I suspect are related to the tempdir sandboxing the macOS sandbox enforces (special per-instance temp path which we don't use in audioipc plus a restriction to "regular" files only).
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
(In reply to Matthew Gregan [:kinetik] from comment #1)
> Would it be possible to pass one end of a socketpair into the content
> process preemptively at process creation time, which the client can then use
> later when trying to create the libcubeb context?  It seems like it'd be
> much simpler to implement, but it does waste an fd in every content process
> that ends up not using audio.

Yes; this is already how we pass down the descriptors for Gecko IPC and the crash reporter (and, on Linux, the rejected syscall reporter).  There are some examples at various places in GeckoChildProcessHost::PerformAsyncLaunchInternal that modify mFileMap (which bug 1401786 will change to mLaunchOptions).

Setting up file descriptors like this isn't supported on Windows, but there might be something else that could be used there.
(In reply to Matthew Gregan [:kinetik] from comment #2)
> Forgot to mention: I ask because I was trying to get audioipc running in
> Firefox on macOS, which should work more-or-less right now, but I'm hitting
> issues with the sandbox that I suspect are related to the tempdir sandboxing
> the macOS sandbox enforces (special per-instance temp path which we don't
> use in audioipc plus a restriction to "regular" files only).

Perhaps not entirely surprisingly, this bug is now blocking similar sandbox improvements on Linux:
https://bugzilla.mozilla.org/show_bug.cgi?id=1386404

If the TMPDIR is changed before the child process launch, to point to the content-process-specific temporary directory, audioipc will fail to locate it's socket and fail.
Duplicate of this bug: 1416228
Priority: P3 → P2
(In reply to Jed Davis [:jld] (⏰UTC-7) from comment #3)
> Setting up file descriptors like this isn't supported on Windows, but there
> might be something else that could be used there.

Good point, and given that we want this to work on Windows soonish, I'm taking an approach along the lines of paragraph 3 of comment 0.  I'll post patches soon.
Blocks: 1425788
Attachment #8937387 - Attachment is obsolete: true
Attachment #8937388 - Attachment description: Bootstrap AudioIPC from Gecko IPC by passing server fd to content process. r?jld,kamidphish → p3: Bootstrap AudioIPC from Gecko IPC by passing server fd to content process. r?jld,kamidphish
Comment on attachment 8937385 [details] [diff] [review]
p1: Update media/audioipc with fd passing changes.  r?kamidphish

Review of attachment 8937385 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/audioipc/client/src/context.rs
@@ +196,5 @@
>              }
>          }
> +        unsafe {
> +            if super::G_SERVER_FD.is_some() {
> +            libc::close(super::G_SERVER_FD.take().unwrap());

nit: indentation
Attachment #8937385 - Flags: review?(dglastonbury) → review+
Attachment #8937386 - Flags: review?(dglastonbury) → review+
Attachment #8937388 - Flags: review?(dglastonbury) → review+
Comment on attachment 8937388 [details] [diff] [review]
p3: Bootstrap AudioIPC from Gecko IPC by passing server fd to content process.  r?jld,kamidphish

Review of attachment 8937388 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall.  Two minor things:

::: dom/media/CubebUtils.cpp
@@ +397,5 @@
> +                  MOZ_ASSERT(!sIPCConnection.IsValid());
> +                  sIPCConnection = aFD;
> +                },
> +                [](mozilla::ipc::ResponseRejectReason aReason) {
> +                  // handle send failure

Should there be something here to at least log the error?

@@ +437,5 @@
> +    // TODO: Don't use audio IPC when within the same process.
> +    MOZ_ASSERT(!sIPCConnection.IsValid());
> +    sIPCConnection = CreateAudioIPCConnection();
> +  } else {
> +    MOZ_ASSERT(sIPCConnection.IsValid());

How certain is it that the SendCreateAudioIPCConnection response will arrive before content can try to use WebAudio?  If we're not entirely sure about that, maybe this should be a MOZ_DIAGNOSTIC_ASSERT to get some coverage on non-debug builds where the timing might be different.
Attachment #8937388 - Flags: review?(jld) → review+
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #14)
> Should there be something here to at least log the error?

Good point, I'll add something when I land.

> @@ +437,5 @@
> > +    // TODO: Don't use audio IPC when within the same process.
> > +    MOZ_ASSERT(!sIPCConnection.IsValid());
> > +    sIPCConnection = CreateAudioIPCConnection();
> > +  } else {
> > +    MOZ_ASSERT(sIPCConnection.IsValid());
> 
> How certain is it that the SendCreateAudioIPCConnection response will arrive
> before content can try to use WebAudio?  If we're not entirely sure about
> that, maybe this should be a MOZ_DIAGNOSTIC_ASSERT to get some coverage on
> non-debug builds where the timing might be different.

I'll switch it to MOZ_DIAGNOSTIC_ASSERT; that's the behaviour I wanted anyway.  I think it'll be fine, but if it turns out to be too late in some situations we can wait on the SendCreateAudioIPCConnection response instead.
(In reply to Dan Glastonbury :kamidphish from comment #13)
> ::: media/audioipc/client/src/context.rs
> @@ +196,5 @@
> >              }
> >          }
> > +        unsafe {
> > +            if super::G_SERVER_FD.is_some() {
> > +            libc::close(super::G_SERVER_FD.take().unwrap());
> 
> nit: indentation

Fixed upstream and rebased patch.
Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de3a554033e6
Update media/audioipc with fd passing changes (2565ddfcacbc14e1ac0d66af1a4154c07050bba2).  r=kamidphish
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fcb75943aa
Update Rust deps for media/audioipc.  r=kamidphish
https://hg.mozilla.org/integration/mozilla-inbound/rev/502337e7c6ae
Bootstrap AudioIPC from Gecko IPC by passing server fd to content process.  r=jld,kamidphish
Jed,

Is this enough for you to start locking down the linux sandbox, or are you still waiting on some more pieces?
Flags: needinfo?(jld)
https://hg.mozilla.org/mozilla-central/rev/de3a554033e6
https://hg.mozilla.org/mozilla-central/rev/a4fcb75943aa
https://hg.mozilla.org/mozilla-central/rev/502337e7c6ae
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I'll test whether this allows tmpdir redirection now.
Flags: needinfo?(jld) → needinfo?(gpascutto)
In local testing, this (+ bug 1394163) allows WebAudio and WebRTC to work with all of my sandboxing work-in-progress turned on (including bug 1401053: it doesn't quite pass Try yet, but the remaining failures don't seem to be media-related).
Depends on: 1426867
Duplicate of this bug: 1416457
Tmpdir redirection landed and didn't have any issues in media tests for a while.
Flags: needinfo?(gpascutto)
You need to log in before you can comment on or make changes to this bug.