From: dan <Dan Kennedy>
Date: Tue, 31 Jan 2023 20:21:06 +0000 (+0000)
Subject: Fix a race condition during hot-journal rollback that could theoretically cause spuri... 
X-Git-Tag: version-3.41.0~71^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fheads%2Fpending-lock-race;p=thirdparty%2Fsqlite.git

Fix a race condition during hot-journal rollback that could theoretically cause spurious corruption errors.

FossilOrigin-Name: 20ea53ddf590a9dd19501fabd2bfdb9c10b5eb265cd2995bdb335769c936c763
---

diff --git a/manifest b/manifest
index 7b52f050f3..4b7e37fff5 100644
--- a/manifest
+++ b/manifest
@@ -1,5 +1,5 @@
-C Additional\stweaks\sto\sthe\senhancement\sat\s[609fbb94b8f01d67]\sto\sfurther\sreduce\nthe\scost\sestimate\sfor\sconstructing\san\sautomatic\sindex\son\san\sephemeral\stable,\nin\sorder\sto\sresolve\sthe\sperformance\sproblem\sdescribed\sby\n[forum:/forumpost/1d571c0296|forum\spost\s1d571c0296].
-D 2023-01-30T20:44:54.399
+C Fix\sa\srace\scondition\sduring\shot-journal\srollback\sthat\scould\stheoretically\scause\sspurious\scorruption\serrors.
+D 2023-01-31T20:21:06.739
 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1
 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea
 F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724
@@ -604,7 +604,7 @@ F src/os.h 1ff5ae51d339d0e30d8a9d814f4b8f8e448169304d83a7ed9db66a65732f3e63
 F src/os_common.h 6c0eb8dd40ef3e12fe585a13e709710267a258e2c8dd1c40b1948a1d14582e06
 F src/os_kv.c 4d39e1f1c180b11162c6dc4aa8ad34053873a639bac6baae23272fc03349986a
 F src/os_setup.h 6011ad7af5db4e05155f385eb3a9b4470688de6f65d6166b8956e58a3d872107
-F src/os_unix.c 9107314500e44908e8d2cdf1ed3333ca5fa17dcfdf9c6a079002b4df90d5f806
+F src/os_unix.c 85096ad0bb4e6a6ac3764603cea0f5332816fa25f0e50c936453edc8ac9acee7
 F src/os_win.c 295fe45f18bd86f2477f4cd79f3377c6f883ceb941b1f46808665c73747f2345
 F src/os_win.h 7b073010f1451abe501be30d12f6bc599824944a
 F src/pager.c fc6d3ec7017d7369ab5dc5421ad1763ff224551c9381866b6da69040db62e406
@@ -677,7 +677,7 @@ F src/test_tclsh.c 7dd98be675a1dc0d1fd302b8247bab992c909db384df054381a2279ad76f9
 F src/test_tclvar.c 33ff42149494a39c5fbb0df3d25d6fafb2f668888e41c0688d07273dcb268dfc
 F src/test_thread.c 269ea9e1fa5828dba550eb26f619aa18aedbc29fd92f8a5f6b93521fbb74a61c
 F src/test_vdbecov.c f60c6f135ec42c0de013a1d5136777aa328a776d33277f92abac648930453d43
-F src/test_vfs.c 2cc38a79892017702d13da79ad5152c196eec19bbd67fbde4d88065aac894a84
+F src/test_vfs.c 193c18da3dbf62a0e33ae7a240bbef938a50846672ee947664512b77d853fe81
 F src/test_vfstrace.c bab9594adc976cbe696ff3970728830b4c5ed698
 F src/test_windirent.c a895e2c068a06644eef91a7f0a32182445a893b9a0f33d0cdb4283dca2486ac1
 F src/test_windirent.h 90dfbe95442c9762357fe128dc7ae3dc199d006de93eb33ba3972e0a90484215
@@ -1371,6 +1371,7 @@ F test/pagesize.test 5769fc62d8c890a83a503f67d47508dfdc543305
 F test/parser1.test 6ccdf5e459a5dc4673d3273dc311a7e9742ca952dd0551a6a6320d27035ce4b3
 F test/pcache.test c8acbedd3b6fd0f9a7ca887a83b11d24a007972b
 F test/pcache2.test af7f3deb1a819f77a6d0d81534e97d1cf62cd442
+F test/pendingrace.test fb997b46d6c144a508fb8025d988a1e511b53d42d24143c57b51de3a405c7490
 F test/percentile.test 4243af26b8f3f4555abe166f723715a1f74c77ff
 F test/permutations.test 4705a032bbfef531bb3fd98b8c6ba4a739998949eae9ac0ea97c8696b331211d
 F test/pg_common.tcl 3b27542224db1e713ae387459b5d117c836a5f6e328846922993b6d2b7640d9f
@@ -2045,8 +2046,11 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93
 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc
 F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e
 F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0
-P 0c2fde767f77d6204e95737edd573f42d72e956a3c20ea7e4daeff906657bbe5
-R eff4b53eb789da902d77064ecbd62787
-U drh
-Z 55e9959d5ae7e5a9913374f5d774ce27
+P bf1aae7a8c7f2c74681aa29baa35259d10ce6a1737d2607def6bf27fed592131
+R acc2b6cc65d6bb367d9e15964ca7789d
+T *branch * pending-lock-race
+T *sym-pending-lock-race *
+T -sym-trunk *
+U dan
+Z 4879461e3eb613f63b1ce94cf45161f5
 # Remove this line to create a well-formed Fossil manifest.
diff --git a/manifest.uuid b/manifest.uuid
index bc5d3b3b4a..9dd108c7c6 100644
--- a/manifest.uuid
+++ b/manifest.uuid
@@ -1 +1 @@
-bf1aae7a8c7f2c74681aa29baa35259d10ce6a1737d2607def6bf27fed592131
\ No newline at end of file
+20ea53ddf590a9dd19501fabd2bfdb9c10b5eb265cd2995bdb335769c936c763
\ No newline at end of file
diff --git a/src/os_unix.c b/src/os_unix.c
index c3359f1bc7..7c1acc8164 100644
--- a/src/os_unix.c
+++ b/src/os_unix.c
@@ -1652,7 +1652,7 @@ static int unixFileLock(unixFile *pFile, struct flock *pLock){
 **    UNLOCKED -> SHARED
 **    SHARED -> RESERVED
 **    SHARED -> (PENDING) -> EXCLUSIVE
-**    RESERVED -> (PENDING) -> EXCLUSIVE
+**    RESERVED -> EXCLUSIVE
 **    PENDING -> EXCLUSIVE
 **
 ** This routine will only increase a lock.  Use the sqlite3OsUnlock()
@@ -1767,7 +1767,7 @@ static int unixLock(sqlite3_file *id, int eFileLock){
   lock.l_len = 1L;
   lock.l_whence = SEEK_SET;
   if( eFileLock==SHARED_LOCK 
-      || (eFileLock==EXCLUSIVE_LOCK && pFile->eFileLock<PENDING_LOCK)
+   || (eFileLock==EXCLUSIVE_LOCK && pFile->eFileLock==RESERVED_LOCK)
   ){
     lock.l_type = (eFileLock==SHARED_LOCK?F_RDLCK:F_WRLCK);
     lock.l_start = PENDING_BYTE;
@@ -1778,6 +1778,9 @@ static int unixLock(sqlite3_file *id, int eFileLock){
         storeLastErrno(pFile, tErrno);
       }
       goto end_lock;
+    }else if( eFileLock==EXCLUSIVE_LOCK ){
+      pFile->eFileLock = PENDING_LOCK;
+      pInode->eFileLock = PENDING_LOCK;
     }
   }
 
@@ -1865,13 +1868,9 @@ static int unixLock(sqlite3_file *id, int eFileLock){
   }
 #endif
 
-
   if( rc==SQLITE_OK ){
     pFile->eFileLock = eFileLock;
     pInode->eFileLock = eFileLock;
-  }else if( eFileLock==EXCLUSIVE_LOCK ){
-    pFile->eFileLock = PENDING_LOCK;
-    pInode->eFileLock = PENDING_LOCK;
   }
 
 end_lock:
diff --git a/src/test_vfs.c b/src/test_vfs.c
index f3e8297ac7..312e1a1be6 100644
--- a/src/test_vfs.c
+++ b/src/test_vfs.c
@@ -485,6 +485,9 @@ static int tvfsLock(sqlite3_file *pFile, int eLock){
     tvfsExecTcl(p, "xLock", Tcl_NewStringObj(pFd->zFilename, -1), 
                    Tcl_NewStringObj(zLock, -1), 0, 0);
   }
+  if( p->mask&TESTVFS_LOCK_MASK && tvfsInjectIoerr(p) ){
+    return SQLITE_IOERR_LOCK;
+  }
   return sqlite3OsLock(pFd->pReal, eLock);
 }
 
@@ -500,7 +503,7 @@ static int tvfsUnlock(sqlite3_file *pFile, int eLock){
     tvfsExecTcl(p, "xUnlock", Tcl_NewStringObj(pFd->zFilename, -1), 
                    Tcl_NewStringObj(zLock, -1), 0, 0);
   }
-  if( p->mask&TESTVFS_WRITE_MASK && tvfsInjectIoerr(p) ){
+  if( p->mask&TESTVFS_UNLOCK_MASK && tvfsInjectIoerr(p) ){
     return SQLITE_IOERR_UNLOCK;
   }
   return sqlite3OsUnlock(pFd->pReal, eLock);
diff --git a/test/pendingrace.test b/test/pendingrace.test
new file mode 100644
index 0000000000..645ebbfc80
--- /dev/null
+++ b/test/pendingrace.test
@@ -0,0 +1,123 @@
+# 2023 January 31
+#
+# The author disclaims copyright to this source code.  In place of
+# a legal notice, here is a blessing:
+#
+#    May you do good and not evil.
+#    May you find forgiveness for yourself and forgive others.
+#    May you share freely, never taking more than you give.
+#
+#***********************************************************************
+#
+
+set testdir [file dirname $argv0]
+source $testdir/tester.tcl
+set testprefix pendingrace
+
+# This test file tests that a race condition surrounding hot-journal
+# rollback that once existed has been resolved. The problem was that
+# if, when attempting to upgrade from a SHARED to EXCLUSIVE lock in
+# order to roll back a hot journal, a connection failed to take the
+# lock, the file-descriptor was left holding a PENDING lock for 
+# a very short amount of time. In a multi-threaded deployment, this
+# could allow a second connection to read the database without rolling
+# back the hot journal.
+#
+
+testvfs tvfs 
+db close
+sqlite3 db test.db -vfs tvfs
+
+# Create a 20 page database using connection [db]. Connection [db] uses
+# Tcl VFS wrapper "tvfs", but it is configured to do straight pass-through
+# for now.
+#
+do_execsql_test 1.0 {
+  PRAGMA cache_size = 5;
+  CREATE TABLE t1(a, b);
+  CREATE INDEX i1 ON t1(a, b);
+  WITH s(i) AS (
+    SELECT 1 UNION ALL SELECT i+1 FROM s WHERE i<10
+  )
+  INSERT INTO t1 SELECT hex(randomblob(100)), hex(randomblob(100)) FROM s;
+  PRAGMA page_count;
+} {20}
+
+# Simulate a crash in another process. This leaves the db with a hot-journal.
+# Without the journal the db is corrupt.
+#
+sqlite3 db2 test.db
+do_execsql_test -db db2 1.1 {
+  PRAGMA cache_size = 5;
+  BEGIN;
+    UPDATE t1 SET b=hex(randomblob(100));
+}
+db_save
+db2 close
+proc my_db_restore {} {
+  forcecopy sv_test.db-journal test.db-journal
+
+  set fd1 [open sv_test.db r]
+  fconfigure $fd1 -encoding binary -translation binary
+  set data [read $fd1]
+  close $fd1
+
+  set fd1 [open test.db w]
+  fconfigure $fd1 -encoding binary -translation binary
+  puts -nonewline $fd1 $data
+  close $fd1
+}
+my_db_restore
+do_test 1.2 {
+  file exists test.db-journal
+} {1}
+
+# Set up connection [db2] to use Tcl VFS wrapper [tvfs2]. Which is configured
+# so that the first call to xUnlock() fails. And then all VFS calls thereafter
+# fail as well.
+#
+testvfs tvfs2
+tvfs2 filter xUnlock
+tvfs2 script xUnlock
+set ::seen_unlock 0
+proc xUnlock {args} {
+  if {$::seen_unlock==0} {
+    set ::seen_unlock 1
+    tvfs2 ioerr 1 1
+    tvfs2 filter {xLock xUnlock}
+  }
+  return ""
+}
+sqlite3 db2 test.db -vfs tvfs2
+
+# Configure [tvfs] (used by [db]) so that within the first call to xAccess,
+# [db2] attempts to read the db. This causes [db2] to fail to upgrade to
+# EXCLUSIVE, leaving it with a PENDING lock. Which it holds on to, 
+# as the xUnlock() and all subsequent VFS calls fail.
+#
+tvfs filter xAccess
+tvfs script xAccess
+set ::seen_access 0
+proc xAccess {args} {
+  if {$::seen_access==0} {
+    set ::seen_access 1
+    catch { db2 eval { SELECT count(*)+0 FROM t1 } }
+    breakpoint
+  }
+  return ""
+}
+
+# Run an integrity check using [db].
+do_catchsql_test 1.3 {
+  PRAGMA integrity_check
+} {1 {database is locked}}
+
+db close
+db2 close
+tvfs delete
+tvfs2 delete
+
+finish_test
+
+
+