Calls to GeckoProfile.get() can result in initialization races

NEW
Unassigned

Status

()

Firefox for Android
General
5 years ago
3 years ago

People

(Reporter: bnicholson, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
See bug 880599, and https://bugzilla.mozilla.org/show_bug.cgi?id=880599#c8 in particular. GeckoProfile.get() should be able to be called anywhere at any time.
(Reporter)

Comment 1

5 years ago
Created attachment 760611 [details] [diff] [review]
Generate a custom profile name when using a custom path

When using a custom profile without specifying a profile name, the fallback profile name is "default". In the profile cache, we store a mapping of profile names -> profile paths. Therefore, using "default" for custom profiles is problematic since we use "default" for the standard generated profile. Whichever path gets set in the cache first is used for all future GeckoProfile.get() calls with the same name.

With this patch, if a custom path is provided with no custom name, a name is generated from the path's hash.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #760611 - Flags: review?(rnewman)
Comment on attachment 760611 [details] [diff] [review]
Generate a custom profile name when using a custom path

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

Sidenote: GeckoApp knowing personal details of the profile is a violation of GeckoProfile's encapsulation; we should aim to kill sIsUsingCustomProfile, profileName, etc. etc. as we move forward, and have GeckoProfile offer those services.

OK, so this patch seems to be an improvement, but how do we know it fixes the issue?

Can you write a test that, e.g.,

  1. Issue a write request to a CP with the default profile. This will mess around with GeckoProfile and do some work.
  2. Initialize GeckoApp for a custom profile.
  3. Verify that before this patch, paths are wrong, and after this patch they are right.

? This certainly strikes me as important enough to have test coverage…

::: mobile/android/base/GeckoApp.java
@@ +1159,3 @@
>                  Matcher m = p.matcher(args);
>                  if (m.find()) {
>                      profilePath =  m.group(1);

s/  / /
Attachment #760611 - Flags: review?(rnewman) → feedback+
(Reporter)

Comment 3

4 years ago
Created attachment 8334754 [details] [diff] [review]
Part 1: Move setting "default" profile into findDefaultProfile

Minor refactoring to contain the default profile logic to GeckoProfile.
Attachment #760611 - Attachment is obsolete: true
Attachment #8334754 - Flags: review?(rnewman)
(Reporter)

Comment 4

4 years ago
Created attachment 8334764 [details] [diff] [review]
Part 2: Robustify profile dir checks

GeckoProfile needs some robustness checks as it currently allows operations with undefined behavior. One such operation currently allowed (but hopefully not occurring anywhere) is multiple calls GeckoProfile.get() with the same profile name and different profile dir. Once we've fetched a profile, we cache it (and its directory) by name, so fetching it again using a different profile dir reassociates the profile with a different directory, mid-session. That's bad.

This also fixes an annoying (but harmless) logging issue that's existed for a few months, where having sync enabled shows gazillions of "requested profile directory missing: null" messages in logcat.
Attachment #8334764 - Flags: review?(rnewman)
(Reporter)

Comment 5

4 years ago
Created attachment 8334765 [details] [diff] [review]
Part 3: Generate a custom profile name when using a custom path

Slightly updated version of the patch posted earlier.
Attachment #8334765 - Flags: review?(rnewman)
(Reporter)

Comment 6

4 years ago
Created attachment 8334781 [details] [diff] [review]
Part 4: Don't lazily create profile

When GeckoProfile.get() is called, we currently store the profile location but wait until GecokProfile.getDir() to actually create the directory. Since mProfileDir is set lazily, this means we need null checks everywhere mProfileDir is used.

Lazily initializing mProfile unnecessarily complicates things since GeckoProfile.getDir() is used fairly frequently in our codebase [1]. That is, any time we fetch a profile, we'll call GeckoProfile.getDir() shortly thereafter anyway. This patch sets mProfileDir to an actual dir whenever the profile is put in the cache, meaning mProfileDir is guaranteed to be non-null.

In general, I'm also being more aggressive about throwing Exceptions in these patches. A failure accessing or creating the profile is a fatal problem, so we shouldn't just catch and ignore these situations.

[1] http://mxr.mozilla.org/mozilla-central/search?string=profile.*getdir&regexp=on&find=java&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Attachment #8334781 - Flags: review?(rnewman)
(Reporter)

Comment 7

4 years ago
Created attachment 8334786 [details] [diff] [review]
Robocop test for profile initialization race

The test profile uses a custom path, so it needs to be different from the default profile. Here's a simple test, as suggested by comment 2, that does a CP insert before the test starts (thus using the default profile), then verifies that the insert has no effect on the test profile.

Without patches: https://tbpl.mozilla.org/?tree=Try&rev=2352f563151b
With patches: https://tbpl.mozilla.org/?tree=Try&rev=29e84ad7decc
Attachment #8334786 - Flags: review?(rnewman)
(Reporter)

Comment 8

4 years ago
Sorry, I got confused uploading these about which patch was which. Part 2 adds only null checks and fixes the logcat issue described in comment 4. The profile-changing-mid-session fix is actually part of patch 4.
(Reporter)

Comment 9

4 years ago
Created attachment 8334853 [details] [diff] [review]
Robocop test for profile initialization race, v2

Minor fixes.
Attachment #8334786 - Attachment is obsolete: true
Attachment #8334786 - Flags: review?(rnewman)
Attachment #8334853 - Flags: review?(rnewman)
(Reporter)

Comment 10

4 years ago
Created attachment 8334855 [details] [diff] [review]
Robocop test for profile initialization race, v3

Sigh. qref'd this time.
Attachment #8334853 - Attachment is obsolete: true
Attachment #8334853 - Flags: review?(rnewman)
Attachment #8334855 - Flags: review?(rnewman)
Heh. "This bug sounds familiar!" -- I reviewed it five months ago! How time flies.

Thanks for the revisions, Brian. On it now.
Comment on attachment 8334754 [details] [diff] [review]
Part 1: Move setting "default" profile into findDefaultProfile

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

::: mobile/android/base/GeckoProfile.java
@@ +489,5 @@
>  
> +        if (sDefaultProfileName == null) {
> +            sDefaultProfileName = "default";
> +        }
> +        return sDefaultProfileName;

I'd probably keep this with a return inside the loop:

for (...) {
    ...
    if (section...) {
        ...
        return sDefaultProfileName = section.getStringProperty("Name");
    }
}

// There must have been no default INI section.
return sDefaultProfileName = "default";
Attachment #8334754 - Flags: review?(rnewman) → review+
Comment on attachment 8334764 [details] [diff] [review]
Part 2: Robustify profile dir checks

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

::: mobile/android/base/GeckoProfile.java
@@ +111,5 @@
>          synchronized (sProfileCache) {
>              GeckoProfile profile = sProfileCache.get(profileName);
>              if (profile == null) {
>                  profile = new GeckoProfile(context, profileName);
>                  profile.setDir(profileDir);

Is this not problematic, too?

@@ +118,1 @@
>                  profile.setDir(profileDir);

... so maybe move this to below the if {} else {} altogether.
Attachment #8334764 - Flags: review?(rnewman) → review+
Comment on attachment 8334765 [details] [diff] [review]
Part 3: Generate a custom profile name when using a custom path

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

::: mobile/android/base/GeckoApp.java
@@ +1162,5 @@
>                      if (m.find()) {
> +                        profilePath = m.group(1);
> +                        if (profileName == null) {
> +                            profileName = GeckoProfile.getProfileNameForPath(profilePath);
> +                        }

I like it.

::: mobile/android/base/GeckoProfile.java
@@ +578,5 @@
>  
>          return profileDir;
>      }
> +
> +    public static String getProfileNameForPath(String path) {

This could do with a Javadoc.

@@ +580,5 @@
>      }
> +
> +    public static String getProfileNameForPath(String path) {
> +        try {
> +            final MessageDigest m = MessageDigest.getInstance("MD5");

TelemetryRecorder does SHA-256. ANRReporter also does. FHR's EnvironmentV1 class and GeckoApp do SHA-1, so those are already initialized. Consider using SHA-1 here, too, unless you have a compelling reason to use MD5... to put in a comment.

@@ +584,5 @@
> +            final MessageDigest m = MessageDigest.getInstance("MD5");
> +            final File f = new File(path);
> +            final String p = f.getCanonicalPath();
> +            m.update(p.getBytes(), 0, p.length());
> +            final BigInteger i = new BigInteger(1, m.digest());

Consider not using BigInteger and String.format.

Two choices.

1. See package org.mozilla.apache.commons.codec.binary, which we ship. This (with Base64 switched for Base32) is essentially what we do in FHR:

  final MessageDigest m = MessageDigest.getInstance("SHA-1");
  final String c = new File(path).getCanonicalPath();
  m.update(path.getBytes("UTF-8"));
  return new Base32(-1, null, false)
               .encodeAsString(m.digest())
               .substring(0, 8);

2. Much easier, though, is this one-liner:

  return org.mozilla.gecko.sync.Utils.sha1Base32(new File(path).getCanonicalPath()).substring(0, 8);
Attachment #8334765 - Flags: review?(rnewman) → review+
Comment on attachment 8334781 [details] [diff] [review]
Part 4: Don't lazily create profile

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

::: mobile/android/base/GeckoProfile.java
@@ +125,2 @@
>                  profile.setDir(profileDir);
> +            } else if (profileDir != null && !profile.getDir().equals(profileDir)) {

Question: why is this an 'else' not simply a follow-on 'if'?

@@ +270,5 @@
>          return false;
>      }
>  
>      public boolean unlock() {
>          // Don't use getDir() as it will create a dir

This comment is no longer true.

@@ +330,5 @@
>              } else {
>                  Log.d(LOGTAG, "Found profile dir: " + mProfileDir.getAbsolutePath());
>              }
> +        } catch (IOException e) {
> +            throw new RuntimeException("Error getting profile dir", e);

I have a gentle preference for this being a checked exception -- simply allow the IOException to propagate upward through the GeckoProfile signatures. This discourages us from ignoring it elsewhere.
Attachment #8334781 - Flags: review?(rnewman) → review+
Comment on attachment 8334855 [details] [diff] [review]
Robocop test for profile initialization race, v3

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

I forget whether this also needs to be added to a manifest. You know better than I do!

f+ so I can look at the new tests.

::: mobile/android/base/tests/testProfile.java
@@ +1,1 @@
> +package org.mozilla.gecko.tests;

License block.

@@ +27,5 @@
> +
> +    @Override
> +    public void setUp() throws Exception {
> +        mBookmarksClass = getInstrumentation().getContext().getClassLoader()
> +                .loadClass("org.mozilla.gecko.db.BrowserContract$Bookmarks");

I thought we didn't need to use reflection for this these days?

@@ +40,5 @@
> +        mMobileFolderId = getMobileFolderId();
> +
> +        // Do a DB write operation before starting the test to verify that the
> +        // ContentProvider uses a different profile.
> +        insertBookmark("Example", "http://example.com");

Over-cautious bastard that I am, I'd almost like two insertBookmark calls here: one which doesn't specify any arguments on mBookmarksUri, and one which explicitly specifies the default profile.

Which brings me to my next point: this is a perfect place to add a tiny, tiny test that verifies that both of those inserted bookmarks ended up in the same profile, which is the default one.

@@ +46,5 @@
> +        super.setUp();
> +    }
> +
> +    public void testProfile() throws Exception {
> +        mAsserter.isnot(mSolo.searchText("Example"), true, "Bookmark not added");

And the consistency test: that when you *do* insert a bookmark into the Robocop profile, using the profile name as the argument in the CP URI, that this check reverses!

The code you've changed could affect name -> profile lookup, so we want to exercise that explicitly.
Attachment #8334855 - Flags: review?(rnewman) → feedback+
Looks good; requested changes inline.

Less-obvious testing for this: make sure that Sync, set up prior to this change, continues to work. Make sure that Guest Mode continues to work -- including that killing the process, starting Guest Mode, adding data elsewhere, and then forcing a sync in Android Settings doesn't add data to the guest profile.
(Reporter)

Comment 18

4 years ago
(In reply to Richard Newman [:rnewman] from comment #12)
> I'd probably keep this with a return inside the loop:
> 
> for (...) {
>     ...
>     if (section...) {
>         ...
>         return sDefaultProfileName = section.getStringProperty("Name");
>     }
> }

I noticed that getStringProperty() can return null, so it seeemed safer to have the null check below that returns "default". Before these patches, callers of findDefaultProfile() would check for null and set the name to "default" on their own; now, that logic is in GeckoProfile, so GeckoProfile is responsible for ensuring the result is non-null.

> TelemetryRecorder does SHA-256. ANRReporter also does. FHR's EnvironmentV1
> class and GeckoApp do SHA-1, so those are already initialized. Consider
> using SHA-1 here, too, unless you have a compelling reason to use MD5... to
> put in a comment.

No reason at all...just picked an arbitrary one. SHA-1 works for me!

> 2. Much easier, though, is this one-liner:
> 
>   return org.mozilla.gecko.sync.Utils.sha1Base32(new
> File(path).getCanonicalPath()).substring(0, 8);

Very nice, thanks.

(In reply to Richard Newman [:rnewman] from comment #15)
> ::: mobile/android/base/GeckoProfile.java
> @@ +125,2 @@
> >                  profile.setDir(profileDir);
> > +            } else if (profileDir != null && !profile.getDir().equals(profileDir)) {
> 
> Question: why is this an 'else' not simply a follow-on 'if'?

The logic goes like this: "If the profile is not in the cache, create a GeckoProfile, add it to the cache, get it directory, and ensure that it exists. Otherwise, if it is in the cache, make sure the profile directory we were just given matches what's cached". The second part only checks whether there's a mismatch between expected and actual values, so if the profile isn't in the cache yet, there's no chance that the given profileDir doesn't match the expected directory: the given profileDir *is* the directory.

> @@ +330,5 @@
> >              } else {
> >                  Log.d(LOGTAG, "Found profile dir: " + mProfileDir.getAbsolutePath());
> >              }
> > +        } catch (IOException e) {
> > +            throw new RuntimeException("Error getting profile dir", e);
> 
> I have a gentle preference for this being a checked exception -- simply
> allow the IOException to propagate upward through the GeckoProfile
> signatures. This discourages us from ignoring it elsewhere.

I can change forceCreate to throw an IOException, though I'm not sure how you want it to be handled from there. forceCreate is called in one place: GeckoProfile#get(Context, String, File). Are you saying we should just have it be converted to a RuntimeException here instead of forceCreate? Or should this method itself throw an IOException? If the latter, that means we'll need to catch an IOException everywhere GeckoProfile#get is called, which is a lot of places. Is there a place we can recover if an IOException is thrown while fetching the profile?

Comments/new patch for test case coming up soon.
(In reply to Brian Nicholson (:bnicholson) from comment #18)

> I noticed that getStringProperty() can return null, so it seeemed safer to
> have the null check below that returns "default".

Fair!


> I can change forceCreate to throw an IOException, though I'm not sure how
> you want it to be handled from there. forceCreate is called in one place:
> GeckoProfile#get(Context, String, File). 

This might be illustrating an important difference that we're not capturing in the code.

Do we always want to force-create a profile when we try to `get` it? Certainly for the ContentProviders we don't -- no sense syncing bookmarks to a profile that doesn't yet exist. And those paths shouldn't need to call forceCreate, and shouldn't throw.

Perhaps we want GeckoProfile.get and GeckoProfile.ensure, the latter of which can throw and the former of which returns null if missing?

> Are you saying we should just have
> it be converted to a RuntimeException here instead of forceCreate? Or should
> this method itself throw an IOException? If the latter, that means we'll
> need to catch an IOException everywhere GeckoProfile#get is called, which is
> a lot of places. Is there a place we can recover if an IOException is thrown
> while fetching the profile?

This is the dilemma of checked exceptions!

I would ideally like every caller of GP.get to be aware that doing so might fail.

We can do that in two ways:

* Use a runtime exception, audit every call site to make sure that we're doing something smart. In some of those call sites there will be an enclosing try block that will silently muffle the failure in a way that's undesirable. In others there won't be, and we'll crash -- perhaps when we don't want to. (For example, we probably don't ever want to crash in a ContentProvider.)

* Use a checked exception, which will *force* us to audit every call site, catch some of those we miss, and to correct all new sites.

From a documentary position, too, by using a RuntimeException we're requiring people to read the docstring or GeckoProfile.java to know that a call might fail.


And finally: regardless of what change we make to the semantics of GeckoProfile.get, we should be updating the call sites, which are currently designed for null checks.
Depends on: 944373
Depends on: 953993
(Reporter)

Updated

3 years ago
Assignee: bnicholson → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.