Closed
Bug 1182565
Opened 9 years ago
Closed 9 years ago
Content process crashes immediately in TSAN build with e10s enabled
Categories
(Core :: Security: Process Sandboxing, defect)
Core
Security: Process Sandboxing
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: chmanchester, Assigned: jld)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
kang
:
review+
chmanchester
:
feedback+
|
Details | Diff | Splinter Review |
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
Updated•9 years ago
|
Component: General → Security: Process Sandboxing
Product: Firefox → Core
Updated•9 years ago
|
tracking-e10s:
--- → ?
Assignee | ||
Comment 1•9 years ago
|
||
…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
Updated•9 years ago
|
Assignee | ||
Comment 2•9 years ago
|
||
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.
Reporter | ||
Comment 3•9 years ago
|
||
I've added --disable-sandbox to the mozconfigs for tsan builds in bug 1181255 for now.
Assignee | ||
Comment 4•9 years ago
|
||
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)
Reporter | ||
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•