Convert multiprocess crash reporter to use pipe() instead of socketpair() in the child

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

(Blocks 1 bug)

Trunk
mozilla35
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

See bug 1066750 for background.  The only message the parent needs to send back to the child is “done dumping”; a pipe works as well as a socket pair for that, and socketpair() has some unwanted features to which we'd like to deny access (which, for unfortunate historical reasons, means blocking all use of socketpair, at least on 32-bit x86).

The FindProcessHoldingSocket code in crash_generation_server has a number of assumptions about socketness, but we can probably remove all of it.  https://crbug.com/357670 covers the removal of the corresponding code from Chromium.  Basically, it's necessary only if a project is doing all of:

1. Using breakpad's remote dumping facility.  (This might only be Gecko, but I think we're not 100% sure of that.)
2. Supporting Linux kernels older than 2.6.35 (released August 2010).
3. Dumping descendent processes in different PID namespaces (e.g., using the Chromium sandbox); note that the affected kernels have recently been desupported by Chromium itself (see link above) and require either running as root or using a setuid-root helper to access this functionality.

We suspect that no non-Gecko projects satisfy all of the above conditions.  B2G could qualify, if we're still using the ICS emulator (kernel 2.6.29, and the only pre-3.0 B2G device) for continuous integration when the process-separation situation improves enough to consider using the full Chromium sandbox — but we control the kernel and can backport the fix, and the B2G sandboxing module owner (namely, me) is fine with that.
Upstream issue for the breakpad part: https://breakpad.appspot.com/1724002/

This also adds pipe(2) to the GMP whitelist and un-conditionalizes it for content processes, so that that will work.  It removes socketpair(2) from the GMP whitelist only, because that syscall has other uses in content processes (see bug 1066750 comment #3).

This could be split into separate patches (allow pipe, change breakpad, disallow socketpair) if need be, but I don't know that that would be useful.
Attachment #8496375 - Flags: review?(ted)
Attachment #8496375 - Flags: review?(gdestuynder)
Comment on attachment 8496373 [details] [diff] [review]
Step 1: remove procfs-searching hack.

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

Patches that do nothing but remove code are pretty nice. :)
Attachment #8496373 - Flags: review?(ted) → review+
Comment on attachment 8496375 [details] [diff] [review]
Step 2: switch crash report client to calling pipe().

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

Boy, that's a remarkably small patch.
Attachment #8496375 - Flags: review?(ted) → review+
Updated with patch for breakpad-patches.  Carrying over r+.
Attachment #8496373 - Attachment is obsolete: true
Attachment #8499854 - Flags: review+
Updated with patch for breakpad-patches.  Carrying over r+.

Thanks for the reviews.
Attachment #8496375 - Attachment is obsolete: true
Attachment #8499856 - Flags: review+
I'll land the Breakpad patches upstream in a bit. ni? myself so I don't forget.
Flags: needinfo?(ted)
https://hg.mozilla.org/mozilla-central/rev/958697c700ca
https://hg.mozilla.org/mozilla-central/rev/afeff2d265bd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Landed upstream:
https://code.google.com/p/google-breakpad/source/detail?r=1389
https://code.google.com/p/google-breakpad/source/detail?r=1390

If you could close those breakpad.appspot.com issues that'd be great.
Flags: needinfo?(ted)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.