Closed Bug 849330 Opened 7 years ago Closed 7 years ago

camera recording needs AID_SDCARD_RW capability

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:leo+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g leo+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: sotaro, Assigned: vliu)

References

Details

Attachments

(2 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #803471 +++

By applying Bug 803471, app process do not need AID_MEDIA/AID_CAMERA permission. Then unprivileged app could access to camera and codec hw. But camra recording still request to an app process to have AID_SDCARD_RW capability.
Depends on: 803471
nsGonkCameraControl::StartRecordingImpl() create a recording file by calling open(). It requests AID_SDCARD_RW.

http://mxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#893
Permissions are assigned to a privileged process in following.

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#305
Summary: After applying Bug 803471, camera recording still needs AID_SDCARD_RW capability → camera recording needs AID_SDCARD_RW capability
So bug 803471 introduced a regression?
(In reply to Mike Habicher [:mikeh] from comment #3)
> So bug 803471 introduced a regression?

This bug is not about a regression. AID_SDCARD_RW is already necessary current Firefox OS 1.0. Bug 803471 removes all necessary privilege except AID_SDCARD_RW.
I see.  So this isn't really a bug then.
Assign to Vincent Liu
--
Keven
Assignee: nobody → vliu
Attached patch Patch file for bug-849330 (obsolete) — Splinter Review
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> (In reply to Mike Habicher [:mikeh] from comment #3)
> > So bug 803471 introduced a regression?
> 
> This bug is not about a regression. AID_SDCARD_RW is already necessary
> current Firefox OS 1.0. Bug 803471 removes all necessary privilege except
> AID_SDCARD_RW.

Hi Sotaro,

Patch was attached removing all necessary privilege except AID_SDCARD_RW. Can you please review it for me? Thanks.
Attachment #723828 - Flags: review?(sotaro.ikeda.g)
Thanks for writing the patch. I am not a correct person to review. cjones and dhylands are correct people to review the patch.

I have comments to the patch.

- PRIVILEGES_VIDEO is not necessary. It can be removed.
http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#312

- "deprecated-hwvideo" can be removed from PrivilegesForApp(mozIApplication* aApp)
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp
(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Thanks for writing the patch. I am not a correct person to review. cjones
> and dhylands are correct people to review the patch.
> 
> I have comments to the patch.
> 
> - PRIVILEGES_VIDEO is not necessary. It can be removed.
> http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/
> process_util_linux.cc#312
> 
> - "deprecated-hwvideo" can be removed from PrivilegesForApp(mozIApplication*
> aApp)
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp

Bug 803471 moves camera hw and hw codec from app process to media server process. By the change, unprivileged app processes can use them.
Taking away AID_AUDIO and AID_CAMERA will just restrict apps needing PRIVLEGES_CAMERA, it won't add anything to unprivileged apps (which agrees with comment 9).

Having said that, we should keep things as restrictive as possible.

Which process plays the shutter noise?
(In reply to Dave Hylands [:dhylands] from comment #10)
> Taking away AID_AUDIO and AID_CAMERA will just restrict apps needing
> PRIVLEGES_CAMERA, it won't add anything to unprivileged apps (which agrees
> with comment 9).
> 
> Having said that, we should keep things as restrictive as possible.
> 
> Which process plays the shutter noise?

Actual audio output to audio hw is done by AudioFlinger in mediaserver process. Camera app start to play shutter sound. Data of the sound is delivered to AudioFlinger via AudioTrack in sydney audio.

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L676

http://mxr.mozilla.org/mozilla-central/source/media/libsydneyaudio/src/sydney_audio_gonk.cpp#237
Comment on attachment 723828 [details] [diff] [review]
Patch file for bug-849330

Review of attachment 723828 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I tested camera and video and both seem to work.
Attachment #723828 - Flags: review?(sotaro.ikeda.g) → review+
I tested using gaia/master against mozilla-inbound on my unagi.
Attached patch patch_v2Splinter Review
(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Thanks for writing the patch. I am not a correct person to review. cjones
> and dhylands are correct people to review the patch.
> 
> I have comments to the patch.
> 
> - PRIVILEGES_VIDEO is not necessary. It can be removed.
> http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/
> process_util_linux.cc#312
> 
> - "deprecated-hwvideo" can be removed from PrivilegesForApp(mozIApplication*
> aApp)
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp

Thanks for your comments. Created v2 patch combined the previous one. 

Hi dhylands,

Can you please review for me?  Thanks.
Attachment #724295 - Flags: review?(dhylands)
Comment on attachment 724295 [details] [diff] [review]
patch_v2

Shouldn't we also remove PRIVLEGES_VIDEO from process_util.h and process_util_linux.cc?

And this wasn't v2 of the patch, it was Part 2 (v2 would have obsoleted an earlier version).
Attachment #724295 - Flags: review?(dhylands) → review+
Attached patch Patch_part2Splinter Review
(In reply to Dave Hylands [:dhylands] from comment #15)
> Comment on attachment 724295 [details] [diff] [review]
> patch_v2
> 
> Shouldn't we also remove PRIVLEGES_VIDEO from process_util.h and
> process_util_linux.cc?
> 
> And this wasn't v2 of the patch, it was Part 2 (v2 would have obsoleted an
> earlier version).

Thanks for the kindly remind of my missing part. The part 2 of the patch was attached.
Attachment #724741 - Flags: review?(dhylands)
Comment on attachment 724741 [details] [diff] [review]
Patch_part2

Review of attachment 724741 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. It looks like you need to obsolete the v1 of the patch.
Attachment #724741 - Flags: review?(dhylands) → review+
Attachment #723828 - Attachment is obsolete: true
Keywords: checkin-needed
Given the useless (and identical) commit messages of the two patches, I went ahead and folded them into one. In the future, commit messages that say what a patch is *doing* is appreciated. Also, please use your full name for commits.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd356da2f14
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6cd356da2f14
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.