Closed
Bug 1181704
Opened 9 years ago
Closed 9 years ago
Linux sandbox logging should be async signal safe
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
References
Details
Attachments
(2 files)
67.02 KB,
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
9.71 KB,
patch
|
kang
:
review+
glandium
:
review+
|
Details | Diff | Splinter Review |
The Linux sandboxing code formats log messages using routines that can allocate memory and otherwise aren't async signal safe, and can do this in async signal context (e.g., if an async signal handler calls open() and thereby raises SIGSYS). So far we've been avoiding problems by doing this only in cases where we're already going to crash, but this isn't ideal. Fortunately, Chromium has a reimplementation of snprintf that's async signal safe. (It's also safer in other ways: it uses C++11 type magic instead of varargs, so format mismatches don't cause memory unsafety, and it can infer buffer size instead of allowing a human to get it wrong.) So we can use that for formatted log messages instead.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8631751 -
Flags: review?(bobowen.code)
Assignee | ||
Comment 2•9 years ago
|
||
r?(glandium) for moz.build changes, which aren't entirely trivial because we're trying to avoid altering the imported Chromium source if possible. r?(kang) for the rest. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd0322a484c (build-only because there aren't really any relevant tests)
Attachment #8631753 -
Flags: review?(mh+mozilla)
Attachment #8631753 -
Flags: review?(gdestuynder)
Comment 3•9 years ago
|
||
Comment on attachment 8631753 [details] [diff] [review] Step 2: Use SafeSPrintf for logging. Review of attachment 8631753 [details] [diff] [review]: ----------------------------------------------------------------- Didn't look at the details of SandboxLogging.cpp. I'll leave that to Guillaume. It may or may not be worth hooking the gtest. If you don't you might as well not import the unittest.cc file in the other patch.
Attachment #8631753 -
Flags: review?(mh+mozilla) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8631751 [details] [diff] [review] Step 1: Import SafeSPrintf. Review of attachment 8631751 [details] [diff] [review]: ----------------------------------------------------------------- Looks like these are from the same version as nearly everything else, so r+ from me. As glandium says, I'm not sure it's worth importing the unit tests when we're not running them, but it's no big deal.
Attachment #8631751 -
Flags: review?(bobowen.code) → review+
Attachment #8631753 -
Flags: review?(gdestuynder) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8864c0587ced https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf7aca43c3a
I had to back this out for static build bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/201c980cabe7 https://treeherder.mozilla.org/logviewer.html#?job_id=11632714&repo=mozilla-inbound
Flags: needinfo?(jld)
Assignee | ||
Comment 7•9 years ago
|
||
Filed bug 1183485 for the static analysis change I'll need to be able to re-land. (The Try run would've caught this if I'd noticed that I needed linux64-st-an in the platform list.)
Flags: needinfo?(jld)
Assignee | ||
Comment 8•9 years ago
|
||
This should be relandable on top of bug 1183485. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eba80eccb55 (The last commit there should only be able to add build bustage, not remove it, so the [S] build passing there means it should also pass without the patch.)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e51c7a34897c https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb5a54331e3
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e51c7a34897c https://hg.mozilla.org/mozilla-central/rev/2fb5a54331e3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•