App ID generation and storage is unsafe in the face of crashes

NEW
Unassigned

Status

()

--
major
6 years ago
5 years ago

People

(Reporter: briansmith, Unassigned)

Tracking

({feature})

unspecified
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:?)

Details

(Whiteboard: [WebAPI:P2])

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

> It would be better to store the max ID in some kind of transactional database
> like IndexedDB or SQLite. Consider the case where the phone powers off (e.g. 
> runs out of battery) before the operating system has flushed the prefs file to
> disk. It would be the same kind of problem that we're trying to fix here. See 
> Bug 775327 comment 3.
blocking-basecamp: + → ?
Whiteboard: [qa-]
If we get this wrong, it can lead to privacy issues.

Mounir, is this something you can do?
Assignee: nobody → mounir
blocking-basecamp: ? → +
Sorry I was muted while we discussed it. The risk here is to reuse the same appId for two different apps. Note that we could mitigate that at startup by checking that the maxLocalId pref is actually higher than the highest appId used by currently installed apps.
(In reply to Fabrice Desré [:fabrice] from comment #2)
> Sorry I was muted while we discussed it. The risk here is to reuse the same
> appId for two different apps. Note that we could mitigate that at startup by
> checking that the maxLocalId pref is actually higher than the highest appId
> used by currently installed apps.

The original bug was that we were using the (highest appId + 1) of the currently-installed app, which causes problems when we've uninstalled an app. If we do what is suggested in #2, we will still be subject to the original bug during the reboot after we crash:

* The pref's value is 6.
* We install an app, which will have appID 6.
* The pref's value becomes 7 in memory.
* We write a bunch of data with appID 6
* That data gets synced to disk
* We uninstall that app
* The max appID in the app file becomes 6 in memory.
* The app file gets synced to disk, so it is 6 on disk too.
* Phone crashes before the prefs file gets synced to disk, so the prefs file still contains maxLocalId=6.
* We reboot and verify that 6 > 5 so everything looks fine, even though it's not.
Whiteboard: [WebAPI:P2]
Keywords: feature
How about we just don't ever decrement the appID?  You can hate me in the event someone ever goes through MAXINT+1 app installs.

Otherwise, this bug sucks but I'm not sure given the straw we have to fit through, that its really a basecamp blocker.  At least I haven't seen traction enough to suggest that's the case.
blocking-basecamp: + → ---
blocking-kilimanjaro: --- → ?
(In reply to Lucas Adamski from comment #4)
> How about we just don't ever decrement the appID?  You can hate me in the
> event someone ever goes through MAXINT+1 app installs.

This is what we do currently, but the bug is about the storage we use to store this ID (a pref).
Blocks: 775293
No longer blocks: 764189
Assignee: mounir → nobody
You need to log in before you can comment on or make changes to this bug.