Content process crashes immediately in TSAN build with e10s enabled

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cmanchester, Assigned: jld)

Tracking

(Blocks 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox42 fixed)

Details

Attachments

(1 attachment)

In the terminal at crash time:

Assertion failure: IsSingleThreaded(), at /home/chris/m-c/security/sandbox/linux/Sandbox.cpp:470
[Parent 20692] WARNING: pipe error (58): Connection reset by peer: file /home/chris/m-c/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459
[Parent 20692] WARNING: pipe error (77): Connection reset by peer: file /home/chris/m-c/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459
[Parent 20692] WARNING: pipe error (79): Connection reset by peer: file /home/chris/m-c/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459

###!!! [Parent][MessageChannel] Error: (msgtype=0x20007D,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

[Parent 20692] WARNING: pipe error (78): Connection reset by peer: file /home/chris/m-c/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459
Component: General → Security: Process Sandboxing
Product: Firefox → Core
…right.  There are some comments in the Chromium source about TSan unavoidably creating extra threads.  We probably just want to disable all sandboxing, and skip that assertion, if TSan is being used.

The simplest way to do that would be for --enable-thread-sanitizer to imply --disable-sandbox, I think.
Assignee: nobody → jld
A few notes:

* This is basically our version of https://crbug.com/461492 except that we don't have unit tests.

* The assertion is applied even if the process in question wouldn't be sandboxed (or if we can't use a sandboxing mechanism where the thread count would matter) so that regressions in whether SandboxEarlyInit is being called in the right place get caught sooner.  That seems to have worked here.

* It would be possible to, as a special case for ThreadSantizer, disable all the namespace sandboxing and this assertion and use seccomp-bpf only — but, if it's anything like AddressSanitizer, it will need some work to adjust the system call policy.  And, unlike with AddressSanitizer, I'm not convinced the benefit of applying it to the sandboxing code is worth the effort.  So I'd rather just disable sandboxing entirely for now; this can be revisited later if necessary.

* Using --disable-sandbox like this will cause problems for bug 1141825, but that can be dealt with in that bug.
I've added --disable-sandbox to the mozconfigs for tsan builds in bug 1181255 for now.
Chris: I don't have a TSan build set up; can you check if this patch works for you?
Attachment #8634865 - Flags: review?(gdestuynder)
Attachment #8634865 - Flags: feedback?(cmanchester)
Comment on attachment 8634865 [details] [diff] [review]
Patch: Disable sandboxing on Linux Thread Sanitizer builds.

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

Yep, I get a usable build with this patch.
Attachment #8634865 - Flags: feedback?(cmanchester) → feedback+
Comment on attachment 8634865 [details] [diff] [review]
Patch: Disable sandboxing on Linux Thread Sanitizer builds.

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

Yeah, given its for the sanitizer builds only it makes sense/easier.
Attachment #8634865 - Flags: review?(gdestuynder) → review+
Thanks for the feedback, and thanks for the review.

For checkin-needed, no Try run because TSan is NPOTB.  I tested a local non-TSan build to double-check that sandboxing still works there.
Keywords: checkin-needed
See Also: → 1141825
https://hg.mozilla.org/mozilla-central/rev/8e1b61112fbb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.