From 8f941bc7a112743c9c12afb02d0ea47e5a8875c3 Mon Sep 17 00:00:00 2001 From: drh Date: Wed, 14 Jan 2009 23:03:40 +0000 Subject: [PATCH] Add asserts to detect if a transaction commits without first incrementing the transaction counter. These asserts are intended to prevent future problems similar to ticket #3584. (CVS 6179) FossilOrigin-Name: b676ccfd9019e65b52251332d94de1b3018ec823 --- manifest | 18 +++++------ manifest.uuid | 2 +- src/os.h | 3 +- src/os_unix.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++- src/pager.c | 12 ++++++- src/sqliteInt.h | 16 +++++++--- 6 files changed, 116 insertions(+), 18 deletions(-) diff --git a/manifest b/manifest index b13e29c689..13b5c01f10 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Fix\stypos\sin\scomments\sin\sFTS3\simplementation.\s(CVS\s6178) -D 2009-01-14T18:59:41 +C Add\sasserts\sto\sdetect\sif\sa\stransaction\scommits\swithout\sfirst\sincrementing\nthe\stransaction\scounter.\s\sThese\sasserts\sare\sintended\sto\sprevent\sfuture\nproblems\ssimilar\sto\sticket\s#3584.\s(CVS\s6179) +D 2009-01-14T23:03:41 F Makefile.arm-wince-mingw32ce-gcc fcd5e9cd67fe88836360bb4f9ef4cb7f8e2fb5a0 F Makefile.in 05461a9b5803d5ad10c79f989801e9fd2cc3e592 F Makefile.linux-gcc d53183f4aa6a9192d249731c90dbdffbd2c68654 @@ -137,12 +137,12 @@ F src/mutex_os2.c 6b5a74f812082a8483c3df05b47bbaac2424b9a0 F src/mutex_unix.c 2f936339dfef1a4c142db290d575a3509b77315f F src/mutex_w32.c ee9cf003a330e2a9d80feefbd2c49899d5d43d6d F src/os.c ed93a6b46132a602c4fd7a58142e2981c829c79d -F src/os.h 4ee25a35c00e75a569cecb5080be299b0c3e208e +F src/os.h f996ab57d2035a20e63173419055608548a834c6 F src/os_common.h 24525d8b7bce66c374dfc1810a6c9043f3359b60 F src/os_os2.c bed77dc26e3a95ce4a204936b9a1ca6fe612fcc5 -F src/os_unix.c 7825c6178597713382d74adbf8c8c00ffcdc42d4 +F src/os_unix.c 5d176993d0f41373d7bc7a6b75f2a536e4efe245 F src/os_win.c 496e3ceb499aedc63622a89ef76f7af2dd902709 -F src/pager.c add3461c2dedf56e56d8d18d3506b6cc322fa363 +F src/pager.c 46ab8721a9fda550a25b1afafa87c66492cf2226 F src/pager.h 9870acb2d653848d90d765d7cbf163496d6c8111 F src/parse.y 4d0e33a702dc3ea7b69d8ae1914b3fbd32e46057 F src/pcache.c 16dc8da6e6ba6250f8dfd9ee46036db1cbceedc6 @@ -158,7 +158,7 @@ F src/select.c ae72b604e47092521c4d9ae54e1b1cbeb872a747 F src/shell.c 65d19f8996a160f288087e31810f24025439c62a F src/sqlite.h.in 6cd2489e40fe97ba58c60044a4ced377e08b6d09 F src/sqlite3ext.h 1db7d63ab5de4b3e6b83dd03d1a4e64fef6d2a17 -F src/sqliteInt.h 10e934f5532a45dcfe9846e1bf4b424b82c5e64d +F src/sqliteInt.h 888bdcaccaa1fde592718357b7f98fccb9974c4a F src/sqliteLimit.h ffe93f5a0c4e7bd13e70cd7bf84cfb5c3465f45d F src/status.c 237b193efae0cf6ac3f0817a208de6c6c6ef6d76 F src/table.c 23db1e5f27c03160987c122a078b4bb51ef0b2f8 @@ -697,7 +697,7 @@ F tool/speedtest16.c c8a9c793df96db7e4933f0852abb7a03d48f2e81 F tool/speedtest2.tcl ee2149167303ba8e95af97873c575c3e0fab58ff F tool/speedtest8.c 2902c46588c40b55661e471d7a86e4dd71a18224 F tool/speedtest8inst1.c 293327bc76823f473684d589a8160bde1f52c14e -P dce60ea764ae50f1008d178029eff5b3959eca38 -R d5048bddda310d85866608242e21adad +P b0f066630c35c4947d3ecd29d32d91036da19e94 +R 0924b91d86d7d36d85d8f1aafd36b93a U drh -Z aa2bc339d080c32f0c3e80a1205aa7c9 +Z 69c19844ae3d053d2d846551c2da1a82 diff --git a/manifest.uuid b/manifest.uuid index 56e7c2bd57..81feca20d2 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -b0f066630c35c4947d3ecd29d32d91036da19e94 \ No newline at end of file +b676ccfd9019e65b52251332d94de1b3018ec823 \ No newline at end of file diff --git a/src/os.h b/src/os.h index 284d96da16..d1ac6b7ac1 100644 --- a/src/os.h +++ b/src/os.h @@ -17,7 +17,7 @@ ** This header file is #include-ed by sqliteInt.h and thus ends up ** being included by every source file. ** -** $Id: os.h,v 1.106 2008/12/08 18:19:18 drh Exp $ +** $Id: os.h,v 1.107 2009/01/14 23:03:41 drh Exp $ */ #ifndef _SQLITE_OS_H_ #define _SQLITE_OS_H_ @@ -245,6 +245,7 @@ int sqlite3OsLock(sqlite3_file*, int); int sqlite3OsUnlock(sqlite3_file*, int); int sqlite3OsCheckReservedLock(sqlite3_file *id, int *pResOut); int sqlite3OsFileControl(sqlite3_file*,int,void*); +#define SQLITE_FCNTL_DB_UNCHANGED 0xca093fa0 int sqlite3OsSectorSize(sqlite3_file *id); int sqlite3OsDeviceCharacteristics(sqlite3_file *id); diff --git a/src/os_unix.c b/src/os_unix.c index 5c7a8a5292..177b7994dc 100644 --- a/src/os_unix.c +++ b/src/os_unix.c @@ -43,7 +43,7 @@ ** * Definitions of sqlite3_vfs objects for all locking methods ** plus implementations of sqlite3_os_init() and sqlite3_os_end(). ** -** $Id: os_unix.c,v 1.235 2009/01/09 21:41:17 drh Exp $ +** $Id: os_unix.c,v 1.236 2009/01/14 23:03:41 drh Exp $ */ #include "sqliteInt.h" #if SQLITE_OS_UNIX /* This file is used on unix only */ @@ -191,6 +191,18 @@ struct unixFile { int isDelete; /* Delete on close if true */ struct vxworksFileId *pId; /* Unique file ID */ #endif +#ifndef NDEBUG + /* The next group of variables are used to track whether or not the + ** transaction counter in bytes 24-27 of database files are updated + ** whenever any part of the database changes. An assertion fault will + ** occur if a file is updated without also updating the transaction + ** counter. This test is made to avoid new problems similar to the + ** one described by ticket #3584. + */ + unsigned char transCntrChng; /* True if the transaction counter changed */ + unsigned char dbUpdate; /* True if any part of database file changed */ + unsigned char inNormalWrite; /* True if in a normal write operation */ +#endif #ifdef SQLITE_TEST /* In test mode, increase the size of this structure a bit so that ** it is larger than the struct CrashFile defined in test6.c. @@ -1318,6 +1330,24 @@ static int unixLock(sqlite3_file *id, int locktype){ } } + +#ifndef NDEBUG + /* Set up the transaction-counter change checking flags when + ** transitioning from a SHARED to a RESERVED lock. The change + ** from SHARED to RESERVED marks the beginning of a normal + ** write operation (not a hot journal rollback). + */ + if( rc==SQLITE_OK + && pFile->locktype<=SHARED_LOCK + && locktype==RESERVED_LOCK + ){ + pFile->transCntrChng = 0; + pFile->dbUpdate = 0; + pFile->inNormalWrite = 1; + } +#endif + + if( rc==SQLITE_OK ){ pFile->locktype = locktype; pLock->locktype = locktype; @@ -1367,6 +1397,23 @@ static int unixUnlock(sqlite3_file *id, int locktype){ SimulateIOErrorBenign(1); SimulateIOError( h=(-1) ) SimulateIOErrorBenign(0); + +#ifndef NDEBUG + /* When reducing a lock such that other processes can start + ** reading the database file again, make sure that the + ** transaction counter was updated if any part of the database + ** file changed. If the transaction counter is not updated, + ** other connections to the same file might not realize that + ** the file has changed and hence might not know to flush their + ** cache. The use of a stale cache can lead to database corruption. + */ + assert( pFile->inNormalWrite==0 + || pFile->dbUpdate==0 + || pFile->transCntrChng==1 ); + pFile->inNormalWrite = 0; +#endif + + if( locktype==SHARED_LOCK ){ lock.l_type = F_RDLCK; lock.l_whence = SEEK_SET; @@ -2699,6 +2746,29 @@ static int unixWrite( int wrote = 0; assert( id ); assert( amt>0 ); + +#ifndef NDEBUG + /* If we are doing a normal write to a database file (as opposed to + ** doing a hot-journal rollback or a write to some file other than a + ** normal database file) then record the fact that the database + ** has changed. If the transaction counter is modified, record that + ** fact too. + */ + if( ((unixFile*)id)->inNormalWrite ){ + unixFile *pFile = (unixFile*)id; + pFile->dbUpdate = 1; /* The database has been modified */ + if( offset<=24 && offset+amt>=27 ){ + char oldCntr[4]; + SimulateIOErrorBenign(1); + seekAndRead(pFile, 24, oldCntr, 4); + SimulateIOErrorBenign(0); + if( memcmp(oldCntr, &((char*)pBuf)[24-offset], 4)!=0 ){ + pFile->transCntrChng = 1; /* The transaction counter has changed */ + } + } + } +#endif + while( amt>0 && (wrote = seekAndWrite((unixFile*)id, offset, pBuf, amt))>0 ){ amt -= wrote; offset += wrote; @@ -2958,6 +3028,17 @@ static int unixFileControl(sqlite3_file *id, int op, void *pArg){ *(int*)pArg = ((unixFile*)id)->lastErrno; return SQLITE_OK; } +#ifndef NDEBUG + /* The pager calls this method to signal that it has done + ** a rollback and that the database is therefore unchanged and + ** it hence it is OK for the transaction change counter to be + ** unchanged. + */ + case SQLITE_FCNTL_DB_UNCHANGED: { + ((unixFile*)id)->dbUpdate = 0; + return SQLITE_OK; + } +#endif #if SQLITE_ENABLE_LOCKING_STYLE && defined(__APPLE__) case SQLITE_SET_LOCKPROXYFILE: case SQLITE_GET_LOCKPROXYFILE: { diff --git a/src/pager.c b/src/pager.c index 55dc04bd5d..81e30bb956 100644 --- a/src/pager.c +++ b/src/pager.c @@ -18,7 +18,7 @@ ** file simultaneously, or one process from reading the database while ** another is writing. ** -** @(#) $Id: pager.c,v 1.550 2009/01/14 17:45:58 danielk1977 Exp $ +** @(#) $Id: pager.c,v 1.551 2009/01/14 23:03:41 drh Exp $ */ #ifndef SQLITE_OMIT_DISKIO #include "sqliteInt.h" @@ -1723,6 +1723,16 @@ static int pager_playback(Pager *pPager, int isHot){ assert( 0 ); end_playback: + /* Following a rollback, the database file should be back in its original + ** state prior to the start of the transaction, so invoke the + ** SQLITE_FCNTL_DB_UNCHANGED file-control method to disable the + ** assertion that the transaction counter was modified. + */ + assert( + pPager->fd->pMethods==0 || + sqlite3OsFileControl(pPager->fd,SQLITE_FCNTL_DB_UNCHANGED,0)>=SQLITE_OK + ); + if( rc==SQLITE_OK ){ zMaster = pPager->pTmpSpace; rc = readMasterJournal(pPager->jfd, zMaster, pPager->pVfs->mxPathname+1); diff --git a/src/sqliteInt.h b/src/sqliteInt.h index bbb824d0c6..fbfa728b05 100644 --- a/src/sqliteInt.h +++ b/src/sqliteInt.h @@ -11,7 +11,7 @@ ************************************************************************* ** Internal interface definitions for SQLite. ** -** @(#) $Id: sqliteInt.h,v 1.823 2009/01/10 16:15:22 drh Exp $ +** @(#) $Id: sqliteInt.h,v 1.824 2009/01/14 23:03:41 drh Exp $ */ #ifndef _SQLITEINT_H_ #define _SQLITEINT_H_ @@ -200,16 +200,22 @@ ** where multiple cases go to the same block of code, testcase() ** can insure that all cases are evaluated. ** -** The TESTONLY macro is used to enclose variable declarations or -** other bits of code that are needed to support the arguments -** within testcase() macros. */ #ifdef SQLITE_COVERAGE_TEST void sqlite3Coverage(int); # define testcase(X) if( X ){ sqlite3Coverage(__LINE__); } -# define TESTONLY(X) X #else # define testcase(X) +#endif + +/* +** The TESTONLY macro is used to enclose variable declarations or +** other bits of code that are needed to support the arguments +** within testcase() and assert() macros. +*/ +#if !defined(NDEBUG) || defined(SQLITE_COVERAGE_TEST) +# define TESTONLY(X) X +#else # define TESTONLY(X) #endif