Closed
Bug 1093893
Opened 10 years ago
Closed 10 years ago
pthread_kill is broken on ICS B2G because the sandbox rejects tkill
Categories
(Core :: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jld, Assigned: jld)
References
Details
Attachments
(1 file, 1 obsolete file)
1.47 KB,
patch
|
jld
:
review+
|
Details | Diff | Splinter Review |
On ICS B2G(/Android) and older, pthread_kill() uses tkill(tid, sig) instead of tgkill(getpid(), tid, sig); the latter is allowed by the current sandbox policy, but not the former. It's simple enough to “polyfill” this by having the SIGSYS handler check for __NR_tkill and do the corresponding tgkill instead. In principle this affects all earlier B2G branches with seccomp sandboxing, but I don't think this needs to be uplifted unless it's blocking something upliftable.
Assignee | ||
Comment 1•10 years ago
|
||
Passes try on top of Luke's patches from bug 1091916: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a5d048ae730 Fails try without this patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=066f76915a23 (i.e., proves that applying those other patches on top of m-c and doing mochitests is sufficient to reproduce the bug)
Attachment #8517653 -
Flags: review?(gdestuynder)
Comment 2•10 years ago
|
||
Thank you so much!
Comment 3•10 years ago
|
||
(Unrelated to this bug) looking at your patch, you #ifdef on "ANDROID_VERSION < 16". I had guess (hopefully wrongly) that we compiled a single binary for Android and thus we'd have to do a dynamic query on the Android version. Is that not the case? Do we compile for several major Android versions and upload the lot of them to the Play store?
Updated•10 years ago
|
Comment 4•10 years ago
|
||
n/m question in comment 3; I was confusing Android version with NDK version.
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #3) > (Unrelated to this bug) looking at your patch, you #ifdef on > "ANDROID_VERSION < 16". I had guess (hopefully wrongly) that we compiled a > single binary for Android and thus we'd have to do a dynamic query on the > Android version. Is that not the case? Do we compile for several major > Android versions and upload the lot of them to the Play store? That might need an a comment. As I understand it, ANDROID_VERSION is the SDK version and thus the lowest version of Android the compiled code will run on. Maybe more importantly, this isn't used on Firefox for Android (yet); we don't have sandboxable child processes there (yet), and we don't have seccomp-bpf support there (yet; Chrome for Android might have plans for it). B2G has seccomp-bpf support because we convinced hardware manufacturers to take kernel patches. B2G, as I understand it, always has ANDROID_VERSION equal to (rather than ≤) the base version for the device.
Comment on attachment 8517653 [details] [diff] [review] bug1093893-ics-tkill-hg0.diff Review of attachment 8517653 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/sandbox/linux/Sandbox.cpp @@ +137,5 @@ > args[3] = SECCOMP_PARM4(ctx); > args[4] = SECCOMP_PARM5(ctx); > args[5] = SECCOMP_PARM6(ctx); > > +#if defined(ANDROID) && ANDROID_VERSION < 16 nit: would be good to have something that indicates which "human readable" version is android 16 (like "4.3") - eventually in this bug, not necessarily in the code
Attachment #8517653 -
Flags: review?(gdestuynder) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Updated with improved comment (API level 16 = Android JB 4.1; https://en.wikipedia.org/wiki/Android_version_history is a useful resource); carrying over r+.
Attachment #8517653 -
Attachment is obsolete: true
Attachment #8518354 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
Try runs in comment #1 should be sufficient.
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2881d59c61f2
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2881d59c61f2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•