1006824 - Fennec can leak profile path in non-standard configurations

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

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

Comment 1

[Nick Alexander :nalexander [he/him]]

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.

Comment 2

[Richard Newman [:rnewman]]

(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.

Comment 3

[Nick Alexander :nalexander [he/him]]

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).

Comment 4

[Matthew Ostrowski]

Attachment: rev1 - fixes profile path leak in non standard configurations

This is my first try at fixing a bug for anything so feedback would be appreciated.
Thanks

Comment 5

[Nick Alexander :nalexander [he/him]]

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.

Comment 6

[Matthew Ostrowski]

Attachment: rev2 - Removed log command line at start-up in official builds. r=nalexander

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.

Comment 7

[Nick Alexander :nalexander [he/him]]

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.)

Comment 8

[Matthew Ostrowski]

Attachment: rev 3 - Removed log command line at start-up in official builds. r=nalexander

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.

Comment 9

[Nick Alexander :nalexander [he/him]]

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.

Comment 10

[Nick Alexander :nalexander [he/him]]

Dear sheriffs: debug logging only, so I'm not concerned about a try build.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/85afdbe47af0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85afdbe47af0