Closed Bug 1006824 Opened 6 years ago Closed 6 years ago

Fennec can leak profile path in non-standard configurations

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: nalexander, Assigned: zonkit71)

Details

(Whiteboard: [mentor=nalexander][lang=java][good first bug])

Attachments

(1 file, 2 obsolete files)

I see a couple of /potential/ profile path leaks in Fennec's startup.  I say potential, since most of the time, we start with "-P default", which leaks nothing.  I wonder if this is an issue with WebApps, mostly because I don't understand how profiles and WebApps interact.

I GeckoThread(978)            RunGecko - args = -no-remote -profile /storage/sdcard0/tests/profile
D GeckoAppShell(978)          GeckoLoader.nativeRun /data/app/org.mozilla.fennec_nalexander-1.apk -greomni /data/app/org.mozilla.fennec_nalexander-1.apk -no-remote -profile /storage/sdcard0/tests/profile -purgecaches -width 1080 -height 1920
I didn't mark this security sensitive since the risk is so low.

I think the correct thing is to only log the arguments in developer builds, or to manually scrub the "-profile ..." part of the args out.  My preference is to log only in developer builds.
Whiteboard: [mentor=nalexander][lang=java][good first bug]
(In reply to Nick Alexander :nalexander from comment #1)

> I think the correct thing is to only log the arguments in developer builds,
> or to manually scrub the "-profile ..." part of the args out.  My preference
> is to log only in developer builds.

Either way, but we should do one of those two things.
To address this bug, find the two places where the arguments are logged (one is at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoThread.java#173, the other is in GeckoAppShell) and add conditional blocks that only log when AppConstants.MOZ_OFFICIAL is false (like at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#330).
This is my first try at fixing a bug for anything so feedback would be appreciated.
Thanks
Attachment #8423083 - Flags: review?(nalexander)
Comment on attachment 8423083 [details] [diff] [review]
rev1 - fixes profile path leak in non standard configurations

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

Hi Matthew, this is looking pretty good.  I've requested a few tweaks so that your changes match the Fennec coding style.  When you upload the next version of your patch, change the commit-line to

Bug 1006824 - Don't log command line at start-up in official builds. r=nalexander

We always try to say what we did, rather than what it is intended to achieve.  The r= part states that I'm the Fennec peer who reviewed your patch.  Thanks!

::: mobile/android/base/GeckoAppShell.java
@@ +349,5 @@
>                  }
>              });
>  
>          // and go
> +        if(!AppConstants.MOZILLA_OFFICIAL){

Fennec coding style has spaces around the braces:

if (!App...) {

::: mobile/android/base/GeckoThread.java
@@ +168,5 @@
>  
>          String args = addCustomProfileArg(mArgs);
>          String type = getTypeFromAction(mAction);
>  
>          // and then fire us up

Let's move the comment down to be before the runGecko call.

@@ +170,5 @@
>          String type = getTypeFromAction(mAction);
>  
>          // and then fire us up
> +        if(!AppConstants.MOZILLA_OFFICIAL){
> +            Log.i(LOGTAG, "RunGecko - args = " + args);

Spaces around the braces here, too.

@@ +176,2 @@
>          GeckoAppShell.runGecko(path, args, mUri, type);
> +        

Delete this extraneous changed line, please.
Attachment #8423083 - Flags: review?(nalexander) → feedback+
I think I fixed everything in question.  Sorry about that. I should have paid more attention to the coding style.  Thank you for your feedback.
Attachment #8423083 - Attachment is obsolete: true
Attachment #8423539 - Flags: review?(nalexander)
Comment on attachment 8423539 [details] [diff] [review]
rev2 - Removed log command line at start-up in official builds.  r=nalexander

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

Formatting and log line look great, but the conditional is reversed throughout :)  (Sorry for the delayed review, it was a holiday weekend here in Canada.)
Attachment #8423539 - Flags: review?(nalexander) → review-
Wow... I do not think I am qualified to be doing this.  I think my first attempt at this will also be my last attempt.  Haha.  I honestly do not know how I missed that.  Too focused on fixing other problems I guess I forgot the entire purpose.  Sorry for kinda wasting your time.  I think this should be golden now.
Attachment #8423539 - Attachment is obsolete: true
Attachment #8425067 - Flags: review?(nalexander)
Comment on attachment 8425067 [details] [diff] [review]
rev 3 - Removed log command line at start-up in official builds. r=nalexander

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

Looks good!  As to coding style and flipping the conditional: that's what first bugs are for!  They're about small changes and learning how we do things, and getting a feel for Bugzilla.  And for learning how the review process works.  As for feeling you're not cut out to contribute to Fennec: you just did!  Now, I want to find you a [good second bug].  I see a small ticket, Bug 956865.  If you're interested, head on over to that bug and I'll see your questions.

Thanks for your help; I hope you'll contribute again; and please feel free to ask me any questions you might have on other tickets.
Attachment #8425067 - Flags: review?(nalexander) → review+
Dear sheriffs: debug logging only, so I'm not concerned about a try build.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/85afdbe47af0
Assignee: nobody → zonkit71
Keywords: checkin-needed
Whiteboard: [mentor=nalexander][lang=java][good first bug] → [mentor=nalexander][lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/85afdbe47af0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=nalexander][lang=java][good first bug][fixed-in-fx-team] → [mentor=nalexander][lang=java][good first bug]
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.