Closed
Bug 776835
Opened 12 years ago
Closed 12 years ago
Apply security checks to PHal
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Core
Hardware Abstraction Layer (HAL)
Tracking
()
People
(Reporter: cjones, Assigned: cjones)
References
(Depends on 1 open bug)
Details
Attachments
(2 files)
(deleted),
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
Some of the interfaces, like setting the system clock, need permissions checks.
Assignee | ||
Comment 1•12 years ago
|
||
This will be very easy to do, but it's going to take me some time to map the Hal interfaces to DOM permissions for the client APIs.
Assignee: nobody → jones.chris.g
Assignee | ||
Comment 2•12 years ago
|
||
(Where such permissions exist ...)
Assignee | ||
Comment 3•12 years ago
|
||
Updated•12 years ago
|
blocking-basecamp: --- → +
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 646422 [details] [diff] [review] Check process capabilities in hal The checks and Permission Names here were derived from https://docs.google.com/spreadsheet/ccc?key=0Akyz_Bqjgf5pdENVekxYRjBTX0dCXzItMnRyUU1RQ0E#gid=0
Attachment #646422 -
Flags: review?(justin.lebar+bug)
Comment 5•12 years ago
|
||
Comment on attachment 646422 [details] [diff] [review] Check process capabilities in hal > virtual bool > RecvSetLight(const LightType& aLight, const hal::LightConfiguration& aConfig, bool *status) MOZ_OVERRIDE > { >+ // XXX we don't currently expose light control independently, but >+ // we do set button backlight on/off along with screen on/off. Nit: Expose light control to what, and independent of what? > virtual bool > RecvEnableSwitchNotifications(const SwitchDevice& aDevice) MOZ_OVERRIDE > { >+ // Content has no reason to listen to switch events currently. >+ switch (aDevice) { >+ SWITCH_HEADPHONES: >+ SWITCH_USB: >+ return false; >+ } >+ > hal::RegisterSwitchObserver(aDevice, this); > return true; > } >@@ -560,16 +629,23 @@ public: > virtual bool > RecvGetCurrentSwitchState(const SwitchDevice& aDevice, hal::SwitchState *aState) MOZ_OVERRIDE > { >+ // Content has no reason to listen to switch events currently. >+ switch (aDevice) { >+ SWITCH_HEADPHONES: >+ SWITCH_USB: >+ return false; >+ } >+ > *aState = hal::GetCurrentSwitchState(aDevice); > return true; > } > }; Isn't this the equivalent of making Recv{Enable,Get}CurrentSwitchState return false unconditionally? (The only other two switch devices are UNKNOWN and NUM_SWITCH_DEVICE.) If so, can we make this less obfuscated?
Attachment #646422 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #5) > Comment on attachment 646422 [details] [diff] [review] > Check process capabilities in hal > > > virtual bool > > RecvSetLight(const LightType& aLight, const hal::LightConfiguration& aConfig, bool *status) MOZ_OVERRIDE > > { > >+ // XXX we don't currently expose light control independently, but > >+ // we do set button backlight on/off along with screen on/off. > > Nit: Expose light control to what, and independent of what? > // XXX currently, the hardware key light and screen backlight are // controlled as a unit. Those are set through the power API, and // there's no other way to poke lights currently, so we require // "power" privileges here. > > virtual bool > > RecvEnableSwitchNotifications(const SwitchDevice& aDevice) MOZ_OVERRIDE > > { > >+ // Content has no reason to listen to switch events currently. > >+ switch (aDevice) { > >+ SWITCH_HEADPHONES: > >+ SWITCH_USB: > >+ return false; > >+ } > >+ > > hal::RegisterSwitchObserver(aDevice, this); > > return true; > > } > >@@ -560,16 +629,23 @@ public: > > virtual bool > > RecvGetCurrentSwitchState(const SwitchDevice& aDevice, hal::SwitchState *aState) MOZ_OVERRIDE > > { > >+ // Content has no reason to listen to switch events currently. > >+ switch (aDevice) { > >+ SWITCH_HEADPHONES: > >+ SWITCH_USB: > >+ return false; > >+ } > >+ > > *aState = hal::GetCurrentSwitchState(aDevice); > > return true; > > } > > }; > > Isn't this the equivalent of making Recv{Enable,Get}CurrentSwitchState > return false unconditionally? (The only other two switch devices are > UNKNOWN and NUM_SWITCH_DEVICE.) If so, can we make this less obfuscated? Yes. Sure.
Assignee | ||
Comment 7•12 years ago
|
||
Failing on try I/Gecko (10188): Protocol error: Handler for EnableWakeLockNotifications returned error code I/Gecko (10188): I/Gecko (10188): ###!!! [Parent][AsyncChannel] Error: Processing error: message was deserialized, but the handler returned false (indicating failure) I/Gecko (10188): why are we enabling wake lock notifications in content processes on android but not b2g ...
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #652334 -
Flags: review?(blassey.bugs)
Comment 9•12 years ago
|
||
Comment on attachment 652334 [details] [diff] [review] Don't try to manage wake locks in android child processes Review of attachment 652334 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsAppShell.cpp @@ +180,5 @@ > { > gAppShell = this; > sAfterPaintListener = new AfterPaintListener(); > > + if (XRE_GetProcessType() == GeckoProcessType_Default) { nit, just return early
Attachment #652334 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Pushed with nit pick https://hg.mozilla.org/integration/mozilla-inbound/rev/9e314d3b7120
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9e314d3b7120
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•