Bug 1108328 (SQLite3.8.7.4)

Upgrade to SQLite 3.8.7.4

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ionnv, Assigned: RyanVM)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
http://www.sqlite.org/news.html

"This release fixes two obscure bugs that can result in incorrect query results and/or application crashes, but not (as far as we can tell) security vulnerabilities. Both bugs have been latent in the code across multiple prior releases and have never before been encountered, so they are unlikely to cause problems. Nevertheless it seems prudent to publish fixes for them both. See the change log for details."

http://www.sqlite.org/releaselog/3_8_7_3.html
Reporter

Updated

5 years ago
Depends on: SQLite3.8.7.2
Assignee

Comment 1

5 years ago
I think we're going to want this on Aurora as well.
Alias: SQLite3.8.7.3
Assignee: nobody → ryanvm
Assignee

Updated

5 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 5

5 years ago
Looks like we omitted a necessary mutex on the 3.8.7.3 change.  Can somebody please do a test run with www.sqlite.org/snapshot/sqlite-amalgamation-201412081745.zip to see if that clears the problem.  (All of the existing SQLite test cases pass, so our own tests are of no help in determining whether or not the problem has been fixed.)

The functional change in the snapshot above is:

--- ../historical/3.8.7.3/sqlite3.c	2014-12-08 10:25:36.223141678 -0500
+++ sqlite3.c	2014-12-08 12:45:20.075284800 -0500
@@ -125896,11 +125896,13 @@
     if( pDb->pBt ){
       if( pDb->pSchema ){
         /* Must clear the KeyInfo cache.  See ticket [e4a18565a36884b00edf] */
+        sqlite3BtreeEnter(pDb->pBt);
         for(i=sqliteHashFirst(&pDb->pSchema->idxHash); i; i=sqliteHashNext(i)){
           Index *pIdx = sqliteHashData(i);
           sqlite3KeyInfoUnref(pIdx->pKeyInfo);
           pIdx->pKeyInfo = 0;
         }
+        sqlite3BtreeLeave(pDb->pBt);
       }
       sqlite3BtreeClose(pDb->pBt);
       pDb->pBt = 0;

So you can just apply the patch above to the 3.8.7.3 SQLite rather than downloading a new amalgamation file, if that is easier.

If the above change works for Firefox, then it will become SQLite version 3.8.7.4 later this week.
Assignee

Comment 6

5 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8cac5df2860e
Alias: SQLite3.8.7.3 → SQLite3.8.7.4
Summary: Upgrade to SQLite 3.8.7.3 → Upgrade to SQLite 3.8.7.4
Assignee

Comment 7

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #6)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8cac5df2860e

No crashes like we were getting with 3.8.7.3. Better yet, Win7 talos appears to like it too!

Comment 8

5 years ago
A source-code audit, based on the prior failure, reveals another place where a mutex is needed, though apparently not a place that is important to FF as the tests are working without this extra mutex.  There is a new snapshot at http://www.sqlite.org/snapshot/sqlite-amalgamation-201412082023.zip which contains both fixes, and also updates the version number to 3.8.7.4.  Ryan, if you can put this new snapshot through the try server once, we'll release it as SQLite 3.8.7.4, probably early tomorrow morning EST.  We'd probably be OK releasing it without the extra try-server run, but we want to avoid having to issue a version 3.8.7.5, so we'd feel better knowing that the new code works well in FF.
Assignee

Comment 10

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #7)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #6)
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8cac5df2860e
> 
> No crashes like we were getting with 3.8.7.3. Better yet, Win7 talos appears
> to like it too!

However, I did manage to hit an instance of bug 1101419 still.
https://bugzilla.mozilla.org/show_bug.cgi?id=1101419

We'll see what the build from comment 8 does.
Assignee

Comment 11

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #9)
> (In reply to D. Richard Hipp from comment #8)
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9362240c9b47
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4e95b0760ebf (PGO
> enabled on supported platforms)

I managed to hit both bug 1103966 and bug 1101419 in these runs, so while version 3.8.7.4 doesn't seem to make things any worse, it doesn't seem to have fixed anything either.

Comment 12

5 years ago
Yeah.  The signatures of but 1103966 and bug 1101419 did not match the problems that 3.8.7.4 fixed.  So it was a long shot.  But worth trying.
Assignee

Comment 13

5 years ago
Attachment #8533185 - Attachment is obsolete: true
Attachment #8533903 - Flags: review?(mak77)
Attachment #8533903 - Flags: review?(mak77) → review+
Assignee

Comment 15

5 years ago
https://hg.mozilla.org/mozilla-central/rev/cff44709cb11
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee

Comment 16

5 years ago
Comment on attachment 8533903 [details] [diff] [review]
Upgrade to SQLite 3.8.7.4

Approval Request Comment
[Feature/regressing bug #]: Pre-existing SQLite bugs
[User impact if declined]: Possible memory corruption and crashes
[Describe test coverage new/current, TBPL]: On trunk and green Try runs and passes SQLite's test suites
[Risks and why]: Low risk, we've already seen with the 3.8.7.3 patches in this bug that our test infra can detect problems caused by this code.
[String/UUID change made/needed]: None
Attachment #8533903 - Flags: approval-mozilla-aurora?
Attachment #8533903 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1109956

Comment 18

5 years ago
Looks like 3.8.8 just got released: http://www.sqlite.org/releaselog/3_8_8.html
Assignee

Updated

5 years ago
You need to log in before you can comment on or make changes to this bug.