Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
216026:
Fix numerous bugs in fts2 where a corrupt fts2 database could cause... (Closed)
|
Created:
11 years, 3 months ago by Chris Evans Modified:
9 years, 6 months ago Reviewers:
Scott Hess - ex-Googler CC:
chromium-reviews_googlegroups.com, huanr, cpu_(ooo_6.6-7.5) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionFix numerous bugs in fts2 where a corrupt fts2 database could cause
out-of-bounds reads and writes.
Apologies for the size of the change but once I got cranking, it was about
half a day to clean up most of the memory errors.
This should not conflict with the in-progress sqlite-3.6.18 upgrade. I diffed
sqlite-3.6.1 vs. sqlite-3.6.18 and the only changes to fts2.c were two spelling
corrections in comments.
These changes are not whimsical -- they do map to crashes we are seeing
in the wild, e.g.
1) Crash in allocator just after leafReaderStep:
http://crash/reportdetail?reportid=303de6d80936c11d
2) Crash in memcpy in leafReaderStep:
http://crash/reportdetail?reportid=6b2c65459042e67c
3) Crash in memcpy in interiorReaderStep:
http://crash/reportdetail?reportid=9f7ba5c321846302
4) Crash in loop in dlrStep trying to read past end of buffer:
http://crash/reportdetail?reportid=61b2eb58397913b2
In addition, this will give relief to users who have (for whatever root cause)
persistent corruption in their databases. Once you get into that state, Chrome
might be unlaunchable or it might crash multiple times per day.
There's a small chance this might help with our biggest crasher, as certain
types of corrupt database could corrupt memory rather than crashing immediately.
BUG=NONE
TEST=Running with tricked out build with assert()s to check for false positives.
Committed revision: http://src.chromium.org/viewvc/chrome?view=rev&revision=27679
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 65
Patch Set 4 : '' #Messages
Total messages: 7 (0 generated)
Chris, thank you for doing this. Negative thanks for making me look at it :-). Has this been run through the relevant SQLite test cases with assertions on? http://codereview.chromium.org/216026/diff/4001/3002 File third_party/sqlite/ext/fts2/fts2.c (right): http://codereview.chromium.org/216026/diff/4001/3002#newcode697 Line 697: if( pReader->nData>0 ){ Throw corrupt on <0 data? http://codereview.chromium.org/216026/diff/4001/3002#newcode709 Line 709: nTotal += n; I think the assertions should remain, here. Just because your code isn't supposed to go off the end of the buffer doesn't mean it won't. Obviously the resolution of assertions is less important now, though, so maybe it's sufficient to have an assertion at the end saying that nTotal didn't pass nData. http://codereview.chromium.org/216026/diff/4001/3002#newcode934 Line 934: int i, n, nTotal; Start nTotal at 0, and always do the nTotal += n idiom. I've found that this kind of code sometimes moves around and the mover (initially) forgets to turn the = into += and vice versa. http://codereview.chromium.org/216026/diff/4001/3002#newcode938 Line 938: if( pReader->nData<=0 ){ pReader->nData<0 implies that we've read past the end of the buffer. I think SQLITE_CORRUPT_BKPT would be better for that case. http://codereview.chromium.org/216026/diff/4001/3002#newcode1213 Line 1213: if( rc!=SQLITE_OK ) goto err; You need plrDestroy(&plReader) somewhere in this case. http://codereview.chromium.org/216026/diff/4001/3002#newcode1297 Line 1297: return rc; In the places where you're making a double-plus-good return, could you please change it to return SQLITE_OK directly? There are a number of these throughout the file. As-is makes me uncomfortable, because this is a known-good return case and most return rc cases are forwarding someone else's return code. http://codereview.chromium.org/216026/diff/4001/3002#newcode3586 Line 3586: rc = dlrStep(&c->reader); Damn, I cannot wrap my head around why the code previously had a dlrStep() between the bind and the return. I can't see that it was important. Maybe it was to make sure that the function always made progress so we didn't end in an infinite loop? But the first error should kill everything, I hope. http://codereview.chromium.org/216026/diff/4001/3002#newcode3650 Line 3650: if( rc!=SQLITE_OK ) break; I don't think you want the break, with will leak new and put a potentialy invalid left in *pResult (that probably doesn't matter). I think you want dataBufferDestroy(&new); return rc; http://codereview.chromium.org/216026/diff/4001/3002#newcode3902 Line 3902: if( rc!=SQLITE_OK ) return rc; queryClear(pQuery); dataBufferDestroy(&new); http://codereview.chromium.org/216026/diff/4001/3002#newcode3913 Line 3913: if( rc!=SQLITE_OK ) return rc; queryClear(pQuery); dataBufferDestroy(&new); http://codereview.chromium.org/216026/diff/4001/3002#newcode3937 Line 3937: if( rc!=SQLITE_OK ) return rc; queryClear(pQuery); dataBufferDestroy(&new); http://codereview.chromium.org/216026/diff/4001/3002#newcode4604 Line 4604: if( nPrefix<0 || nPrefix>pReader->term.nData ) return SQLITE_CORRUPT_BKPT; nSuffix>pReader->nData for consistency in reading. http://codereview.chromium.org/216026/diff/4001/3002#newcode5193 Line 5193: if( nPrefix<0 || nPrefix>pReader->term.nData ) return SQLITE_CORRUPT_BKPT; Make nSuffix>pReader->nData for consistency. http://codereview.chromium.org/216026/diff/4001/3002#newcode5316 Line 5316: if( rc!=SQLITE_OK ) return rc; The caller will not call leavesReaderDestroy() in this case, so you need to call dataBufferDestroy(&pReader->rootData) before returning. http://codereview.chromium.org/216026/diff/4001/3002#newcode5403 Line 5403: if( rc!=SQLITE_OK ) return rc; The code will later call leafReaderDestroy(&pReader->leafReader). Instead of this, probably should call leafReaderInit() on a stack local, and on success destroy pReader->leafReader and copy the new one in. http://codereview.chromium.org/216026/diff/4001/3002#newcode5518 Line 5518: if( pData == NULL ){ no spaces around ==. http://codereview.chromium.org/216026/diff/4001/3002#newcode5686 Line 5686: if( pData == NULL ){ No space around ==. http://codereview.chromium.org/216026/diff/4001/3002#newcode5737 Line 5737: if( rc!= SQLITE_OK ) goto err; No space after !=. http://codereview.chromium.org/216026/diff/4001/3002#newcode5742 Line 5742: if( rc!= SQLITE_OK ) goto err; Here too. http://codereview.chromium.org/216026/diff/4001/3002#newcode5850 Line 5850: if( rc!=SQLITE_OK ) return rc; interiorReaderDestroy(&reader); http://codereview.chromium.org/216026/diff/4001/3002#newcode5861 Line 5861: if( rc!=SQLITE_OK ) return rc; interiorReaderDestroy(&reader); http://codereview.chromium.org/216026/diff/4001/3002#newcode5918 Line 5918: if( rc!=SQLITE_OK ) return rc; Since you're iterating a statement, you need the sqlite3_reset(s) line from above. http://codereview.chromium.org/216026/diff/4001/3002#newcode6033 Line 6033: dlrDestroy(&readers[1]); This chunk might be cleaner with nested if()s. Like rc = dlrInit(&...0); if( rc==SQLITE_OK ){ rc = dlrInit(&...1); if( rc==SQLITE_OK ){ dataBufferInit(); rc = docListMerge(); dataBufferDestroy(); *out = merge; dlrDestroy(&...1); } dlrDestroy(&...0); } http://codereview.chromium.org/216026/diff/4001/3002#newcode6088 Line 6088: } Instead of the goto in this case, use an else. http://codereview.chromium.org/216026/diff/4001/3002#newcode6457 Line 6457: if( pData == NULL ){ no spaces around ==. http://codereview.chromium.org/216026/diff/4001/3002#newcode6521 Line 6521: dlrDestroy(&dlReaders[0]); Your goto err cases above leak this guy. http://codereview.chromium.org/216026/diff/4001/3002#newcode6536 Line 6536: if( rc!=SQLITE_OK ) break; I see that this is correct, but it's kinda confusing having the other calls be goto err. If you really want this to be break, you should also convert the check after docListTrim() above. http://codereview.chromium.org/216026/diff/4001/3002#newcode6870 Line 6870: dataBufferInit(&dump, 0); Push dataBufferInit down so it isn't initialized if dlrInit() fails. http://codereview.chromium.org/216026/diff/4001/3002#newcode6873 Line 6873: for( ; !dlrAtEnd(&dlReader); rc = dlrStep(&dlReader) ){ rc==SQLITE_OK && !dlrAtEnd() here... http://codereview.chromium.org/216026/diff/4001/3002#newcode6876 Line 6876: if( rc!=SQLITE_OK ) break; ...so you can remove this line. http://codereview.chromium.org/216026/diff/4001/3002#newcode6920 Line 6920: dlrDestroy(&dlReader); if rc!=SQLITE_OK, DataBufferDestroy(&dump) and return here. dump.nData could be zero.
On 2009/09/22 21:03:41, shess wrote: > Chris, thank you for doing this. Negative thanks for making me look at it :-). > > Has this been run through the relevant SQLite test cases with assertions on? By this I mean sqlite/tests/fts*. Chrome's tests don't tests that SQLite works right, they test that Chrome bits work right with what SQLite gives them.
On Tue, Sep 22, 2009 at 2:11 PM, <shess@chromium.org> wrote: > On 2009/09/22 21:03:41, shess wrote: > >> Chris, thank you for doing this. Negative thanks for making me look at it >> > :-). > > Has this been run through the relevant SQLite test cases with assertions >> on? >> > > By this I mean sqlite/tests/fts*. Chrome's tests don't tests that SQLite > works > right, they test that Chrome bits work right with what SQLite gives them. I'm happy to do this. I use Linux. I have a built Chromium source tree. What is the command-line to build and run the relevant tests? Cheers Chris > > http://codereview.chromium.org/216026 >
I haven't done that in over a year, so your guess is as good as mine. Look at http://codereview.chromium.org/209058 and see if that gets you there. If not, please help Mike capture the knowledge in reproducible form! -scott On Tue, Sep 22, 2009 at 2:39 PM, Chris Evans <cevans@google.com> wrote: > On Tue, Sep 22, 2009 at 2:11 PM, <shess@chromium.org> wrote: >> >> On 2009/09/22 21:03:41, shess wrote: >>> >>> Chris, thank you for doing this. =A0Negative thanks for making me look = at >>> it >> >> :-). >> >>> Has this been run through the relevant SQLite test cases with assertion= s >>> on? >> >> By this I mean sqlite/tests/fts*. =A0Chrome's tests don't tests that SQL= ite >> works >> right, they test that Chrome bits work right with what SQLite gives them= . > > I'm happy to do this. > I use Linux. I have a built Chromium source tree. What is the command-lin= e > to build and run the relevant tests? > > Cheers > Chris >> >> >> http://codereview.chromium.org/216026 > >
Thank you Scott for such a thorough review! Can I get a final look? I hereby also promise to re-run the fts2 tests before submitting. http://codereview.chromium.org/216026/diff/4001/3002 File third_party/sqlite/ext/fts2/fts2.c (right): http://codereview.chromium.org/216026/diff/4001/3002#newcode697 Line 697: if( pReader->nData>0 ){ On 2009/09/22 21:03:41, shess wrote: > Throw corrupt on <0 data? This really is a "can't happen" condition with my change so I prefer to leave it to the asserts. http://codereview.chromium.org/216026/diff/4001/3002#newcode709 Line 709: nTotal += n; On 2009/09/22 21:03:41, shess wrote: > I think the assertions should remain, here. Just because your code isn't > supposed to go off the end of the buffer doesn't mean it won't. Obviously the > resolution of assertions is less important now, though, so maybe it's sufficient > to have an assertion at the end saying that nTotal didn't pass nData. Done. I like your idea of the new assert at the end. http://codereview.chromium.org/216026/diff/4001/3002#newcode934 Line 934: int i, n, nTotal; On 2009/09/22 21:03:41, shess wrote: > Start nTotal at 0, and always do the nTotal += n idiom. I've found that this > kind of code sometimes moves around and the mover (initially) forgets to turn > the = into += and vice versa. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode938 Line 938: if( pReader->nData<=0 ){ On 2009/09/22 21:03:41, shess wrote: > pReader->nData<0 implies that we've read past the end of the buffer. I think > SQLITE_CORRUPT_BKPT would be better for that case. It's now a "can't happen" condition. I added asserts back as per previous suggestion. http://codereview.chromium.org/216026/diff/4001/3002#newcode1213 Line 1213: if( rc!=SQLITE_OK ) goto err; On 2009/09/22 21:03:41, shess wrote: > You need plrDestroy(&plReader) somewhere in this case. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode1297 Line 1297: return rc; On 2009/09/22 21:03:41, shess wrote: > In the places where you're making a double-plus-good return, could you please > change it to return SQLITE_OK directly? There are a number of these throughout > the file. As-is makes me uncomfortable, because this is a known-good return > case and most return rc cases are forwarding someone else's return code. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode3650 Line 3650: if( rc!=SQLITE_OK ) break; On 2009/09/22 21:03:41, shess wrote: > I don't think you want the break, with will leak new and put a potentialy > invalid left in *pResult (that probably doesn't matter). I think you want > dataBufferDestroy(&new); return rc; Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode3902 Line 3902: if( rc!=SQLITE_OK ) return rc; On 2009/09/22 21:03:41, shess wrote: > queryClear(pQuery); dataBufferDestroy(&new); Done. Actually - did "if( i!=nNot ) dataBufferDestroy(&left);" as per other error paths too. http://codereview.chromium.org/216026/diff/4001/3002#newcode3913 Line 3913: if( rc!=SQLITE_OK ) return rc; On 2009/09/22 21:03:41, shess wrote: > queryClear(pQuery); dataBufferDestroy(&new); Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode3937 Line 3937: if( rc!=SQLITE_OK ) return rc; On 2009/09/22 21:03:41, shess wrote: > queryClear(pQuery); dataBufferDestroy(&new); Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode4604 Line 4604: if( nPrefix<0 || nPrefix>pReader->term.nData ) return SQLITE_CORRUPT_BKPT; On 2009/09/22 21:03:41, shess wrote: > nSuffix>pReader->nData for consistency in reading. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5193 Line 5193: if( nPrefix<0 || nPrefix>pReader->term.nData ) return SQLITE_CORRUPT_BKPT; On 2009/09/22 21:03:41, shess wrote: > Make nSuffix>pReader->nData for consistency. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5316 Line 5316: if( rc!=SQLITE_OK ) return rc; On 2009/09/22 21:03:41, shess wrote: > The caller will not call leavesReaderDestroy() in this case, so you need to call > dataBufferDestroy(&pReader->rootData) before returning. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5403 Line 5403: if( rc!=SQLITE_OK ) return rc; On 2009/09/22 21:03:41, shess wrote: > The code will later call leafReaderDestroy(&pReader->leafReader). Instead of > this, probably should call leafReaderInit() on a stack local, and on success > destroy pReader->leafReader and copy the new one in. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5518 Line 5518: if( pData == NULL ){ On 2009/09/22 21:03:41, shess wrote: > no spaces around ==. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5686 Line 5686: if( pData == NULL ){ On 2009/09/22 21:03:41, shess wrote: > No space around ==. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5737 Line 5737: if( rc!= SQLITE_OK ) goto err; On 2009/09/22 21:03:41, shess wrote: > No space after !=. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5742 Line 5742: if( rc!= SQLITE_OK ) goto err; On 2009/09/22 21:03:41, shess wrote: > Here too. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5850 Line 5850: if( rc!=SQLITE_OK ) return rc; On 2009/09/22 21:03:41, shess wrote: > interiorReaderDestroy(&reader); Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5861 Line 5861: if( rc!=SQLITE_OK ) return rc; On 2009/09/22 21:03:41, shess wrote: > interiorReaderDestroy(&reader); Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode5918 Line 5918: if( rc!=SQLITE_OK ) return rc; On 2009/09/22 21:03:41, shess wrote: > Since you're iterating a statement, you need the sqlite3_reset(s) line from > above. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode6033 Line 6033: dlrDestroy(&readers[1]); On 2009/09/22 21:03:41, shess wrote: > This chunk might be cleaner with nested if()s. Like > > rc = dlrInit(&...0); > if( rc==SQLITE_OK ){ > rc = dlrInit(&...1); > if( rc==SQLITE_OK ){ > dataBufferInit(); > rc = docListMerge(); > dataBufferDestroy(); > *out = merge; > dlrDestroy(&...1); > } > dlrDestroy(&...0); > } > Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode6088 Line 6088: } On 2009/09/22 21:03:41, shess wrote: > Instead of the goto in this case, use an else. Done. By moving "rc = SQLITE_OK" up, don't even need an else. http://codereview.chromium.org/216026/diff/4001/3002#newcode6457 Line 6457: if( pData == NULL ){ On 2009/09/22 21:03:41, shess wrote: > no spaces around ==. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode6521 Line 6521: dlrDestroy(&dlReaders[0]); On 2009/09/22 21:03:41, shess wrote: > Your goto err cases above leak this guy. I don't see that they do? The first "goto err" is just after a while loop which blasts them all. The second "goto err" is the case where dlrInit() failed and so dlrDestroy() should not be needed. http://codereview.chromium.org/216026/diff/4001/3002#newcode6536 Line 6536: if( rc!=SQLITE_OK ) break; On 2009/09/22 21:03:41, shess wrote: > I see that this is correct, but it's kinda confusing having the other calls be > goto err. If you really want this to be break, you should also convert the > check after docListTrim() above. Done. Agree use of "break" for deep nesting like this is very unclear. Switched to "goto err". http://codereview.chromium.org/216026/diff/4001/3002#newcode6870 Line 6870: dataBufferInit(&dump, 0); On 2009/09/22 21:03:41, shess wrote: > Push dataBufferInit down so it isn't initialized if dlrInit() fails. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode6873 Line 6873: for( ; !dlrAtEnd(&dlReader); rc = dlrStep(&dlReader) ){ On 2009/09/22 21:03:41, shess wrote: > rc==SQLITE_OK && !dlrAtEnd() here... Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode6876 Line 6876: if( rc!=SQLITE_OK ) break; On 2009/09/22 21:03:41, shess wrote: > ...so you can remove this line. Done. http://codereview.chromium.org/216026/diff/4001/3002#newcode6920 Line 6920: dlrDestroy(&dlReader); On 2009/09/22 21:03:41, shess wrote: > if rc!=SQLITE_OK, DataBufferDestroy(&dump) and return here. dump.nData could be > zero. Done.
LGTM. I just don't have the gumption to give this a ground-up re-review. Your changes address my concerns from the first pass. I have butterflies in my tummy. Trust the SQLite tests, I guess! http://codereview.chromium.org/216026/diff/4001/3002 File third_party/sqlite/ext/fts2/fts2.c (right): http://codereview.chromium.org/216026/diff/4001/3002#newcode697 Line 697: if( pReader->nData>0 ){ On 2009/09/28 23:19:04, Chris Evans wrote: > On 2009/09/22 21:03:41, shess wrote: > > Throw corrupt on <0 data? > This really is a "can't happen" condition with my change so I prefer to leave it > to the asserts. OK. http://codereview.chromium.org/216026/diff/4001/3002#newcode3902 Line 3902: if( rc!=SQLITE_OK ) return rc; On 2009/09/28 23:19:04, Chris Evans wrote: > On 2009/09/22 21:03:41, shess wrote: > > queryClear(pQuery); dataBufferDestroy(&new); > > Done. Actually - did "if( i!=nNot ) dataBufferDestroy(&left);" as per other > error paths too. I think this is the right thing to do, here, but it's really opaque code. I often wished for a scoped_whatever<>() when writing this code. [OK, I didn't write this specific code, but you get my point!] http://codereview.chromium.org/216026/diff/4001/3002#newcode6088 Line 6088: } On 2009/09/28 23:19:04, Chris Evans wrote: > On 2009/09/22 21:03:41, shess wrote: > > Instead of the goto in this case, use an else. > > Done. By moving "rc = SQLITE_OK" up, don't even need an else. Excellent! http://codereview.chromium.org/216026/diff/4001/3002#newcode6521 Line 6521: dlrDestroy(&dlReaders[0]); On 2009/09/28 23:19:04, Chris Evans wrote: > On 2009/09/22 21:03:41, shess wrote: > > Your goto err cases above leak this guy. > > I don't see that they do? The first "goto err" is just after a while loop which > blasts them all. The second "goto err" is the case where dlrInit() failed and so > dlrDestroy() should not be needed. You're right! I think. Who wrote this code, anyhow? |
