Closed Bug 1632687 Opened 5 months ago Closed 2 months ago

Remove "channel IDs" on Unix

Categories

(Core :: IPC, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: jld, Assigned: jld)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

The Chromium-derived IPC code was, as far as I can tell, originally designed for Windows and assumed that channels would be named pipes, managed and connected to via std::wstring paths. The port to Unix, however, used unnamed socketpair() and passed them directly from process to process, so it has no use for these channel IDs... but it still computes and propagates them, even though they're not used, using deprecated wide-string APIs.

This also indirectly causes bug 1556599, because using wide strings causes glibc locale initialization which reads the environment on the I/O thread, presumably racing with something on the main thread changing the environment. Removing channel IDs isn't a proper fix for the underlying problem there — we'd need to refrain from modifying the environment while multithreaded, which is a large task and outside the scope of IPC — but it will prevent this particular instance of it.

The PipeMap class tries to simulate the Windows channel model (named
pipes that the client opens by a pathname) on Unix. However, it's
effectively dead code -- the map is empty except in some unit tests that
we never imported.

What we do is generate a "channel ID" with string formatting, then don't
pass it to the child or ever insert anything into the map, then the child
looks up an empty string and doesn't find it, so it uses the hard-coded
fixed fd for the initial channel.

Basically, it does nothing except maybe confuse unfamiliar readers, so
let's get rid of it.

This "create a pipe" operation has a mode where, on Unix, it doesn't
create a new transport but rather uses a hard-coded fd for the initial
IPC channel in a child process. (It was originally written for Windows
and the assumption of using named pipes and pathnames for everything.)

That seems like a footgun, so this patch checks for trying to "create"
that pipe twice. However, it doesn't check for accidentally calling it
in the parent process.

The Chromium-derived IPC code was, as far as I can tell, originally
designed for Windows and assumed that channels would be named pipes,
managed and connected to via std::wstring paths. The port to Unix,
however, used unnamed socketpair() and passed them directly from
process to process, so it has no use for these channel IDs... but it
still computes and propagates them, even though they're not used, using
deprecated wide-string APIs.

This patch introduces a typedef for an abstract channel ID, which is a
wstring on Windows and an empty struct on Unix, to allow removing the
string code where it's not needed without needing to completely redesign
the channel abstraction.

Attachment #9142904 - Attachment description: Bug 1632687 - Part 2: Introduce an OS-dependent ChannelId type to reflect that Unix doesn't use channel IDs. → Bug 1632687 - Part 3: Introduce an OS-dependent ChannelId type to reflect that Unix doesn't use channel IDs.

Chromium's fix for CVE-2011-3079 added an optional prefix parameter for
channel IDs, but we've never used it and have no plans to. (Chromium
itself doesn't appear to have used it except with the prefixes "gpu"
and "nacl", and the code has since been removed completely in favor of
Mojo.) So let's simplify things and remove it.

Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1706ed209e22
Part 1: Remove IPC PipeMap. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/9cdc37802d12
Part 1.5: Protect the hard-coded IPC child fd from accidental multiple use. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/66bfa0dd136d
Part 2: Remove the channel ID prefixes, which we've never used. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/d87f5e311934
Part 3: Introduce an OS-dependent ChannelId type to reflect that Unix doesn't use channel IDs. r=mccr8
You need to log in before you can comment on or make changes to this bug.