Closed Bug 591447 Opened 9 years ago Closed 9 years ago

Cookie rowids may collide if PR_Now() winds backward

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dwitte, Assigned: dwitte)

Details

(Whiteboard: [has review])

Attachments

(5 files, 1 obsolete file)

We have code to ensure cookie rowid monotonically increases. This relies on knowing the rowid of every cookie in the db at the time a cookie is added, so we can keep track of the max current rowid and generate a new one appropriately.

This could break if we don't read in everything synchronously on startup, as is now the case. There's a tiny tiny chance that a new rowid could collide with an existing one if the system clock gets wound back. We want to make the chance of this happening zero.
Any kind of cookie corruption is a horrible, horrible thing, and it's a simple fix, so it should probably block.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
I kinda fixed this while I was in there; we weren't doing a great job of keeping track of what state we're operating on. For instance, there's nothing wrong with switching into PB mode but keeping an async read going in the background; the data should be read into the default state. It's important to fix this.
Attachment #476960 - Flags: review?(sdwilsh)
Attached patch part 2: add triple index (obsolete) — Splinter Review
The meat. This adds a creationTime column and decouples the id. We still need the GenerateUniqueCreationTime goo to enforce monotonicity when adding multiple cookies; the only difference is we don't need to guarantee uniqueness now.

This also means that we need to enforce uniqueness on (name, host, path), which requires changing FindCookie() to not get smart about expiry time. I took the opportunity to reflect this change in the API behavior (see TestCookie change), which I think is saner than sneakily pretending that expired cookies never existed and not notifying about their deletion.

With this patch, remove operations take about 26% longer on the async thread, which is no big deal.
Attachment #476979 - Flags: review?(sdwilsh)
Somewhat unrelated to this bug, but this improves our eviction tests to ensure persistence as well. (This is relevant because PurgeCookies() has its own transaction batching code that doesn't otherwise get exercised.)

Also a couple other nitfixes in the cookieservice proper.
Attachment #477277 - Flags: review?(sdwilsh)
Comment on attachment 476960 [details] [diff] [review]
part 1: fix mDefaultDBState

r=sdwilsh
Attachment #476960 - Flags: review?(sdwilsh) → review+
75% done with the review of part 2, but I think we want as much testing on this as possible from real users, so making this a beta 8 blocker.
blocking2.0: betaN+ → beta8+
Comment on attachment 476979 [details] [diff] [review]
part 2: add triple index

>+++ b/netwerk/cookie/nsCookie.h
>+    // Generate a unique creation time. See comment in nsCookie.cpp.
>+    static PRInt64 GenerateUniqueCreationTime(PRInt64 aCreationTime);
Probably useful to say it's a unique monotonically increasing time here.  The rest of the details can be found in nsCookie.cpp, but I think the monotonically increasing bit is important.

>+++ b/netwerk/cookie/nsCookieService.cpp
>+        // Update the schema version.
>+        rv = mDBState->dbConn->SetSchemaVersion(COOKIES_SCHEMA_VERSION);
>+        NS_ENSURE_SUCCESS(rv, rv);
Not sure this is actually what we want to do.  While it's true that if we have an error, the transaction will roll us back (with my next comment addressed at least), it feels rather odd to set the schema version to something it clearly is not at this point.  We don't really need this at all, but we could set it at the very last one.

>+    case 3:
>+      {
>+        mozStorageTransaction transaction(mDBState->dbConn, PR_TRUE);
In reality, we should have the entire else block (http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#747) in one transaction here.

>+        // Compute the baseDomains for the table. This must be done eagerly
>+        // otherwise we won't be able to synchronously read in individual
>+        // domains on demand.
This comment is bogus, yeah?

>+        nsCOMPtr<mozIStorageStatement> update;
>+        rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
>+          "UPDATE moz_cookies SET creationTime = ?1 WHERE id = ?1"),
Use named parameters please.  Yes it's slightly slower ( O(1) vs O(log(n)) ), but n is *very* small here, and we are limited by disk speed anyway.

>+        nsCString baseDomain, host;
not used

>+        PRBool hasResult;
>+        while (1) {
>+          rv = select->ExecuteStep(&hasResult);
>+          NS_ENSURE_SUCCESS(rv, rv);
>+
>+          if (!hasResult)
>+            break;
This is usually written as |while (NS_SUCCEDDED(select->ExecuteStep(&hasResult)) && hasResult) {| which is far more compact

>+        // Update the schema version.
>+        rv = mDBState->dbConn->SetSchemaVersion(COOKIES_SCHEMA_VERSION);
>+        NS_ENSURE_SUCCESS(rv, rv);
This should be moved outside of the case brace, and placed right above case COOKIES_SCHEMA_VERSION so it's the last thing done for all migration.  Nuke it from other spots.

>   // cache frequently used statements (for insertion, deletion, and updating)
>   rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
>     "INSERT INTO moz_cookies ("
>-      "id, "
>       "baseDomain, "
>       "name, "
>       "value, "
>       "host, "
>       "path, "
>       "expiry, "
>       "lastAccessed, "
>+      "creationTime, "
>       "isSecure, "
>       "isHttpOnly"
>     ") VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)"),
You might hate me, but since we need to change all of our queries anyway, we should move to named columns to make the code about a million times more readable.

>   rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
>-    "DELETE FROM moz_cookies WHERE id = ?1"),
>+    "DELETE FROM moz_cookies "
>+    "WHERE name = ?1 AND host = ?2 AND path = ?3"),
>     getter_AddRefs(mDBState->stmtDelete));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = mDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
>-    "UPDATE moz_cookies SET lastAccessed = ?1 WHERE id = ?2"),
>+    "UPDATE moz_cookies SET lastAccessed = ?1 "
>+    "WHERE name = ?2 AND host = ?3 AND path = ?4"),
>     getter_AddRefs(mDBState->stmtUpdate));
>   NS_ENSURE_SUCCESS(rv, rv);
...or at the very least, let's get these two

>@@ -953,18 +1013,20 @@ nsCookieService::CreateTable()
>       "id INTEGER PRIMARY KEY, "
>       "baseDomain TEXT, "
>       "name TEXT, "
>       "value TEXT, "
>       "host TEXT, "
>       "path TEXT, "
>       "expiry INTEGER, "
>       "lastAccessed INTEGER, "
>+      "creationTime INTEGER, "
>       "isSecure INTEGER, "
>-      "isHttpOnly INTEGER"
>+      "isHttpOnly INTEGER, "
>+      "UNIQUE (name, host, path)"
You name it in the migration path, so we should do that here too:
http://sqlite.org/syntaxdiagrams.html#table-constraint

>-    // if the old cookie is httponly, make sure we're not coming from script
>-    if (!aFromHttp && oldCookie->IsHttpOnly()) {
>+    // If the old cookie is httponly, make sure we're not coming from script --
>+    // but, if the old cookie has already expired, pretend like it didn't exist.
>+    if (!aFromHttp && oldCookie->IsHttpOnly() &&
>+        oldCookie->Expiry() > currentTime) {
>       COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "previously stored cookie is httponly; coming from script");
>+      // If the cookie 
Something tells me you didn't proofread your patch :P

This looks pretty solid though.  r=sdwilsh with comment addressed.
Attachment #476979 - Flags: review?(sdwilsh) → review+
Comment on attachment 477277 [details] [diff] [review]
test improvements

r=sdwilsh
Attachment #477277 - Flags: review?(sdwilsh) → review+
Whiteboard: [has review][needs new patch]
(In reply to comment #7)
> Not sure this is actually what we want to do.

Yeah, copy/paste mistake. I meant it to be the right number :(

> >+        while (1) {
> >+          rv = select->ExecuteStep(&hasResult);
> >+          NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+          if (!hasResult)
> >+            break;
> This is usually written as |while
> (NS_SUCCEDDED(select->ExecuteStep(&hasResult)) && hasResult) {| which is far
> more compact

But I want it to return immediately if ExecuteStep fails.

> Something tells me you didn't proofread your patch :P

I fixed that locally about 30 seconds after uploading it. It was a test to see if you'd spot it :)

Will fix other comments.
blocking2.0: beta8+ → ---
blocking2.0: --- → beta8+
(In reply to comment #7)
> You might hate me, but since we need to change all of our queries anyway, we
> should move to named columns to make the code about a million times more
> readable.

Doing so costs about 1.1% in main-thread statement speed (presumably the named params are looked up on the main thread and bound as indices), 80.7us --> 81.7us. So I'm fine with it.
For posterity.
Attachment #476979 - Attachment is obsolete: true
Attachment #478103 - Flags: review+
Whiteboard: [has review][needs new patch] → [has review]
Possible that this is causing cookie failure when restarting the browser and you find that your logged out of forums?
You're testing an hourly build from a few hours ago? What rev?

Does the problem persist if you log back into the forums and restart?
Yes, was using cset http://hg.mozilla.org/mozilla-central/rev/1fdd1f2ef8f9 hourly build.

Logging back into the forums and restarting, followed by 'refresh' of the tab would leave me logged out again.
FWIW, cookies.sqlite "date modified" in Explorer didn't change after first broken build start, despite several shutdowns and restarts, until the backed-out build fixed things.
blocking2.0: beta8+ → betaN+
This is necessary to prevent failure when creating a unique index. (In schema version 3, we can have multiple cookies with the same (name, host, path) values, where all but one is expired.) It's a bunch of code, but there's no prettier way to do it that I can think of.

On the bright side, this also simplifies the creationTime migration code -- turns out it's trivial to do as a SQL statement.

Comprehensive tests for all this migration stuff will be over in bug 599799.
Attachment #483049 - Flags: review?(sdwilsh)
Now that we're enforcing uniqueness on (name, host, path), we need to make sure we eagerly replace an expired cookie when we try to set one that would be nonunique.

This also changes some of the methods to allow enumeration of expired cookies. This is necessary for tests -- we want to be able to test whether expired cookies are preserved or otherwise during migration and such.

There's also a bonus fix to RemoveAll() that cancels an in-flight async read. (Otherwise bad things would happen!)
Attachment #483051 - Flags: review?(sdwilsh)
Comment on attachment 483049 [details] [diff] [review]
part 3: delete expired cookies on migrate

Ugh, migration is going to be slow, but there is not a better way.

r=sdwilsh
Attachment #483049 - Flags: review?(sdwilsh) → review+
Comment on attachment 483051 [details] [diff] [review]
part 4: replace expired cookies eagerly

nit: brace your one line for loops please

r=sdwilsh
Attachment #483051 - Flags: review?(sdwilsh) → review+
You need to log in before you can comment on or make changes to this bug.