Closed Bug 1355273 Opened 3 years ago Closed 3 years ago

Allow inotify_init along with inotify_init1

Categories

(Core :: Security: Process Sandboxing, defect)

Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jld, Assigned: jld)

Details

(Whiteboard: sblc2)

Crash Data

Attachments

(1 file)

I'm seeing some SIGSYS crashes on amd64 with the inotify_init syscall.  We allow inotify_init1, which is inotify_init extended to take a flags argument; according to the man page, inotify_init() is equivalent to inotify_init1(0).

We'll probably need to do something about inotify at some point, but until then, it's a one-line change to fix this discrepancy.
Assignee: nobody → jld
Whiteboard: sblc3
Comment on attachment 8858458 [details]
Bug 1355273 - Allow inotify_init alongside inotify_init1.

https://reviewboard.mozilla.org/r/130422/#review133348
Attachment #8858458 - Flags: review+
Comment on attachment 8858458 [details]
Bug 1355273 - Allow inotify_init alongside inotify_init1.

https://reviewboard.mozilla.org/r/130422/#review133880
Attachment #8858458 - Flags: review?(gpascutto) → review+
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eded510b0f3b
Allow inotify_init alongside inotify_init1. r=Alex_Gaynor,gcp
https://hg.mozilla.org/mozilla-central/rev/eded510b0f3b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: sblc3 → sblc2
Comment on attachment 8858458 [details]
Bug 1355273 - Allow inotify_init alongside inotify_init1.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1337162
[User impact if declined]: Unclear; could cause problems with external protocol handlers on older Linux distributions.
[Is this code covered by automated tests?]: Yes and no — there are automated tests that indirectly call inotify_init(), but on our CI that uses the inotify_init1 syscall which was already allowed.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This patch just allows an older version of a system call that we're already allowing; it makes Firefox on older Linux distributions match what we're doing on newer ones, and it makes 54's behavior closer to 53.
[String changes made/needed]: None


Additional background: the only place we use inotify seems to be indirectly via nsGNOMERegistry and the "gio" library, in the context of Necko checking for external applications that handle protocol schemes.  It's not clear what effect denying access to inotify would have; it doesn't seem to break any of our automated tests, but that's no guarantee that all real use cases are unaffected.
Attachment #8858458 - Flags: approval-mozilla-beta?
Comment on attachment 8858458 [details]
Bug 1355273 - Allow inotify_init alongside inotify_init1.

This patch makes Firefox on older Linux distributions match what we're doing on newer ones. Beta54+. Should be in 54 beta 10.
Attachment #8858458 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #6)
> [Is this code covered by automated tests?]: Yes and no — there are automated
> tests that indirectly call inotify_init(), but on our CI that uses the
> inotify_init1 syscall which was already allowed.
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Jed's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.