From: dan Date: Wed, 9 Dec 2015 20:05:27 +0000 (+0000) Subject: Update sqlite3_snapshot_open() to reduce the chances of reading a corrupt snapshot... X-Git-Tag: version-3.10.0~44^2~12 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=65127cd57d20a2dc302dc2cc51a389f1089bad13;p=thirdparty%2Fsqlite.git Update sqlite3_snapshot_open() to reduce the chances of reading a corrupt snapshot created by a checkpointer process exiting unexpectedly. FossilOrigin-Name: 7315f7cbf4179aadda0f1a0baa1526a9b9f9729f --- diff --git a/manifest b/manifest index f06c0af37a..09955e2042 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Merge\sunrelated\sfixes\sfrom\strunk. -D 2015-12-09T16:04:06.348 +C Update\ssqlite3_snapshot_open()\sto\sreduce\sthe\schances\sof\sreading\sa\scorrupt\ssnapshot\screated\sby\sa\scheckpointer\sprocess\sexiting\sunexpectedly. +D 2015-12-09T20:05:27.534 F Makefile.in 28bcd6149e050dff35d4dcfd97e890cd387a499d F Makefile.linux-gcc 7bc79876b875010e8c8f9502eb935ca92aa3c434 F Makefile.msc e8fdca1cb89a1b58b5f4d3a130ea9a3d28cb314d @@ -341,7 +341,7 @@ F src/resolve.c a83b41104e6ff69855d03cd0aaa09e93927ec39f F src/rowset.c eccf6af6d620aaa4579bd3b72c1b6395d9e9fa1e F src/select.c f8fded11fc443a9f5a73cc5db069d06b34460e2f F src/shell.c abbc74ea43dbf2f306ea18282d666683fb5efab2 -F src/sqlite.h.in fc8a2875a318df1b9dabd82cb00b1ac98081423a +F src/sqlite.h.in 19dea4862ccfcc1a733d0fd18d4744b02a505ac6 F src/sqlite3.rc 992c9f5fb8285ae285d6be28240a7e8d3a7f2bad F src/sqlite3ext.h dfbe62ffd95b99afe2140d8c35b180d11924072d F src/sqliteInt.h 5caacf37a776f9d6178e519cb0b5248ca22a3828 @@ -415,7 +415,7 @@ F src/vdbesort.c a7ec02da4494c59dfd071126dd3726be5a11459d F src/vdbetrace.c 8befe829faff6d9e6f6e4dee5a7d3f85cc85f1a0 F src/vtab.c 2a8b44aa372c33f6154208e7a7f6c44254549806 F src/vxworks.h c18586c8edc1bddbc15c004fa16aeb1e1342b4fb -F src/wal.c abce669053edf5cd1cd1751d654d48d74ed47839 +F src/wal.c 0bd8aa8e0db924493af4c72f527afc9b9e22257a F src/wal.h 907943dfdef10b583e81906679a347e0ec6f1b1b F src/walker.c 2e14d17f592d176b6dc879c33fbdec4fbccaa2ba F src/where.c b18edbb9e5afabb77f4f27550c471c5c824e0fe7 @@ -1020,7 +1020,7 @@ F test/skipscan2.test d1d1450952b7275f0b0a3a981f0230532743951a F test/skipscan3.test ec5bab3f81c7038b43450e7b3062e04a198bdbb5 F test/skipscan5.test 67817a4b6857c47e0e33ba3e506da6f23ef68de2 F test/skipscan6.test 5866039d03a56f5bd0b3d172a012074a1d90a15b -F test/snapshot.test 061dc75b77ca65c0e9c5976499625abe5be7a5c0 +F test/snapshot.test 800e0be4488acb88dd38ff9e9b83edb71d9d5a9d F test/soak.test 0b5b6375c9f4110c828070b826b3b4b0bb65cd5f F test/softheap1.test 843cd84db9891b2d01b9ab64cef3e9020f98d087 F test/sort.test 3f492e5b7be1d3f756728d2ff6edf4f6091e84cb @@ -1409,7 +1409,7 @@ F tool/vdbe_profile.tcl 246d0da094856d72d2c12efec03250d71639d19f F tool/warnings-clang.sh f6aa929dc20ef1f856af04a730772f59283631d4 F tool/warnings.sh 48bd54594752d5be3337f12c72f28d2080cb630b F tool/win/sqlite.vsix deb315d026cc8400325c5863eef847784a219a2f -P 502cc6f353358946080d9bcd335aed526825b88a 901d0b8f3b72e96ffa8e9436993a12980f5ebd51 -R 9b1d7228446872f8d72f41cfd12e515e -U drh -Z 3ae5744205bfcf5cc63a2630633fbd47 +P 362615b4df94358d0264b0991c3090a0878f054c +R cfc950fbd468350b7777662be2b401a3 +U dan +Z 51de2f0ad57bbe117dc864890e31b83c diff --git a/manifest.uuid b/manifest.uuid index 44bc42da2d..33aefc5d3b 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -362615b4df94358d0264b0991c3090a0878f054c \ No newline at end of file +7315f7cbf4179aadda0f1a0baa1526a9b9f9729f \ No newline at end of file diff --git a/src/sqlite.h.in b/src/sqlite.h.in index e8940b5e08..601415b907 100644 --- a/src/sqlite.h.in +++ b/src/sqlite.h.in @@ -7895,11 +7895,39 @@ int sqlite3_db_cacheflush(sqlite3*); ** SQLITE_ERROR is returned. If any other error occurs, for example an IO ** error or an OOM condition, the corresponding SQLite error code is ** returned. +** +** Each successful call to sqlite3_snapshot_get() must be matched by a call +** to sqlite3_snapshot_free() to delete the snapshot handle. Not doing so +** is a memory leak. The results of using a snapshot handle after it has +** been deleted by sqlite3_snapshot_free() are undefined. +** +** Given a snapshot handle, the sqlite3_snapshot_open() API function may be +** used to open a read transaction on the same database snapshot that was +** being read when sqlite3_snapshot_get() was called to obtain it. The +** combination of the first two arguments to sqlite3_snapshot_open() - a +** database handle and the name (e.g. "main") of one of its attached +** databases - must refer to the same database file as that identified by +** the arguments passed to the sqlite3_snapshot_get() call. The database +** handle must not have an open read or write transaction on this database +** file, and must not be in auto-commit mode. +** +** An old database snapshot may only be opened if SQLite is able to +** determine that it is still valid. The only way for an application to +** guarantee that a snapshot remains valid is by holding an open +** read-transaction on it or on an older snapshot of the same database +** file. If SQLite cannot determine that the snapshot identified by the +** snapshot handle, SQLITE_BUSY_SNAPSHOT is returned. +** +** Otherwise, if the read transaction is successfully opened, SQLITE_OK is +** returned. If the named database is not in wal mode or if the database +** handle already has an open read or write transaction on it, or if the +** database handle is in auto-commit mode, SQLITE_ERROR is returned. If +** an OOM or IO error occurs, the associated SQLite error code is returned. */ typedef struct sqlite3_snapshot sqlite3_snapshot; int sqlite3_snapshot_get(sqlite3*, const char*, sqlite3_snapshot **ppSnapshot); -int sqlite3_snapshot_open(sqlite3*, const char*, sqlite3_snapshot*); void sqlite3_snapshot_free(sqlite3_snapshot*); +int sqlite3_snapshot_open(sqlite3*, const char*, sqlite3_snapshot*); /* ** Undo the hack that converts floating point types to integer for diff --git a/src/wal.c b/src/wal.c index 49811fb9b4..af93694e8d 100644 --- a/src/wal.c +++ b/src/wal.c @@ -2275,6 +2275,9 @@ static int walTryBeginRead(Wal *pWal, int *pChanged, int useWal, int cnt){ { if( (pWal->readOnly & WAL_SHM_RDONLY)==0 && (mxReadMarkpSnapshot==0 +#endif ){ for(i=1; ipSnapshot ) return SQLITE_BUSY_SNAPSHOT; +#endif assert( rc==SQLITE_BUSY || (pWal->readOnly & WAL_SHM_RDONLY)!=0 ); return rc==SQLITE_BUSY ? WAL_RETRY : SQLITE_READONLY_CANTLOCK; } @@ -2383,22 +2389,54 @@ int sqlite3WalBeginReadTransaction(Wal *pWal, int *pChanged){ #ifdef SQLITE_ENABLE_SNAPSHOT if( rc==SQLITE_OK ){ if( pSnapshot && memcmp(pSnapshot, &pWal->hdr, sizeof(WalIndexHdr)) ){ + /* At this point the client has a lock on an aReadMark[] slot holding + ** a value equal to or smaller than pSnapshot->mxFrame. This client + ** did not populate the aReadMark[] slot. pWal->hdr is populated with + ** the wal-index header for the snapshot currently at the head of the + ** wal file, which is different from pSnapshot. + ** + ** The presence of the aReadMark[] slot entry makes it very likely + ** that either there is currently another read-transaction open on + ** pSnapshot, or that there has been one more recently than the last + ** checkpoint of any frames greater than pSnapshot->mxFrame was + ** started. There is an exception though: client 1 may have called + ** walTryBeginRead and started to open snapshot pSnapshot, setting + ** the aReadMark[] slot to do so. At the same time, client 2 may + ** have committed a new snapshot to disk and started a checkpoint. + ** In this circumstance client 1 does not end up reading pSnapshot, + ** but may leave the aReadMark[] slot populated. + ** + ** The race condition above is difficult to detect. One approach would + ** be to check the aReadMark[] slot for another client. But this is + ** prone to false-positives from other snapshot clients. And there + ** is no equivalent to xCheckReservedLock() for wal locks. Another + ** approach would be to take the checkpointer lock and check that + ** fewer than pSnapshot->mxFrame frames have been checkpointed. But + ** that does not account for checkpointer processes that failed after + ** checkpointing frames but before updating WalCkptInfo.nBackfill. + ** And it would mean that this function would block on checkpointers + ** and vice versa. + ** + ** TODO: For now, this race condition is ignored. + */ volatile WalCkptInfo *pInfo = walCkptInfo(pWal); - rc = walLockShared(pWal, WAL_READ_LOCK(0)); - if( rc==SQLITE_OK ){ - if( pInfo->nBackfill<=pSnapshot->mxFrame - && pSnapshot->aSalt[0]==pWal->hdr.aSalt[0] - && pSnapshot->aSalt[1]==pWal->hdr.aSalt[1] - ){ - assert( pWal->readLock>0 ); - assert( pInfo->aReadMark[pWal->readLock]<=pSnapshot->mxFrame ); - memcpy(&pWal->hdr, pSnapshot, sizeof(WalIndexHdr)); - *pChanged = bChanged; - }else{ - rc = SQLITE_BUSY_SNAPSHOT; - } - walUnlockShared(pWal, WAL_READ_LOCK(0)); + + assert( pWal->readLock>0 ); + assert( pInfo->aReadMark[pWal->readLock]<=pSnapshot->mxFrame ); + + /* Check that the wal file has not been wrapped. Assuming it has not, + ** overwrite pWal->hdr with *pSnapshot and set *pChanged as appropriate + ** for opening the snapshot. Or, if the wal file has been wrapped + ** since pSnapshot was written, return SQLITE_BUSY_SNAPSHOT. */ + if( pSnapshot->aSalt[0]==pWal->hdr.aSalt[0] + && pSnapshot->aSalt[1]==pWal->hdr.aSalt[1] + ){ + memcpy(&pWal->hdr, pSnapshot, sizeof(WalIndexHdr)); + *pChanged = bChanged; + }else{ + rc = SQLITE_BUSY_SNAPSHOT; } + if( rc!=SQLITE_OK ){ sqlite3WalEndReadTransaction(pWal); } diff --git a/test/snapshot.test b/test/snapshot.test index 4d94225701..5d3758c010 100644 --- a/test/snapshot.test +++ b/test/snapshot.test @@ -57,12 +57,14 @@ do_execsql_test 1.3.2 COMMIT # Check that a simple case works. Reuse the database created by the # block of tests above. # -do_execsql_test 2.0 { +# UPDATE: This case (2.1) no longer works. 2.2 does. +# +do_execsql_test 2.1.0 { BEGIN; SELECT * FROM t1; } {1 2 3 4 5 6 7 8} -do_test 2.1 { +do_test 2.1.1 { set snapshot [sqlite3_snapshot_get db main] execsql { COMMIT; @@ -71,15 +73,45 @@ do_test 2.1 { } } {1 2 3 4 5 6 7 8 9 10} -do_test 2.2 { +do_test 2.1.2 { + execsql BEGIN + list [catch { sqlite3_snapshot_open db main $snapshot } msg] $msg +} {1 SQLITE_BUSY_SNAPSHOT} + +do_test 2.1.3 { + sqlite3_snapshot_free $snapshot + execsql COMMIT +} {} + +do_test 2.2.0 { + sqlite3 db2 test.db + execsql { + BEGIN; + SELECT * FROM t1; + } db2 +} {1 2 3 4 5 6 7 8 9 10} + +do_test 2.2.1 { + set snapshot [sqlite3_snapshot_get db2 main] + execsql { + INSERT INTO t1 VALUES(11, 12); + SELECT * FROM t1; + } +} {1 2 3 4 5 6 7 8 9 10 11 12} + +do_test 2.1.2 { execsql BEGIN sqlite3_snapshot_open db main $snapshot - execsql { SELECT * FROM t1 } -} {1 2 3 4 5 6 7 8} + execsql { + SELECT * FROM t1; + } +} {1 2 3 4 5 6 7 8 9 10} -do_test 2.3 { +do_test 2.1.3 { sqlite3_snapshot_free $snapshot execsql COMMIT + execsql COMMIT db2 + db2 close } {} #-------------------------------------------------------------------------