591447 - Cookie rowids may collide if PR_Now() winds backward

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

Any kind of cookie corruption is a horrible, horrible thing, and it's a simple fix, so it should probably block.

Comment 2

[[e-mail]]

Attachment: part 1: fix mDefaultDBState

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.

Comment 3

[[e-mail]]

Attachment: part 2: add triple index

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.

Comment 4

[[e-mail]]

Attachment: test improvements

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.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 476960 [details] [diff] [review]
part 1: fix mDefaultDBState

r=sdwilsh

Comment 6

[Shawn Wilsher :sdwilsh]

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.

Comment 7

[Shawn Wilsher :sdwilsh]

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.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 477277 [details] [diff] [review]
test improvements

r=sdwilsh

Comment 9

[[e-mail]]

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

Comment 10

[[e-mail]]

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

Comment 11

[[e-mail]]

Attachment: part 2: add triple index

For posterity.

Comment 12

[Jim Jeffery not reading bug-mail 1/2/11]

Possible that this is causing cookie failure when restarting the browser and you find that your logged out of forums?

Comment 13

[[e-mail]]

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?

Comment 14

[Jim Jeffery not reading bug-mail 1/2/11]

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.

Comment 15

[bomfog]

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.

Comment 16

[[e-mail]]

Attachment: part 3: delete expired cookies on migrate

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.

Comment 17

[[e-mail]]

Attachment: part 4: replace expired cookies eagerly

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

Comment 18

[Shawn Wilsher :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

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 483051 [details] [diff] [review]
part 4: replace expired cookies eagerly

nit: brace your one line for loops please

r=sdwilsh

Comment 20

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/7eff93151b2d
http://hg.mozilla.org/mozilla-central/rev/03f4d774f862
http://hg.mozilla.org/mozilla-central/rev/d980bbd3d675
http://hg.mozilla.org/mozilla-central/rev/48fabf7bca4b
http://hg.mozilla.org/mozilla-central/rev/9fdcc7db7957

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Could this have caused bug 605835?