Do not set pMethods when xOpen fails (a failed open closes an unrelated live file)

failedopen_test.go

@@ -0,0 +1,79 @@
+package sqlite3vfs
+
+import (
+	"database/sql"
+	"fmt"
+	"testing"
+
+	_ "github.com/mattn/go-sqlite3"
+)
+
+// recordingVFS remembers every file it opens.
+type recordingVFS struct {
+	*TmpVFS
+	opened []*TmpFile
+}
+
+func (vfs *recordingVFS) Open(name string, flags OpenFlag) (File, OpenFlag, error) {
+	f, retFlags, err := vfs.TmpVFS.Open(name, flags)
+	if err != nil {
+		return nil, 0, err
+	}
+
+	tf := f.(*TmpFile)
+	vfs.opened = append(vfs.opened, tf)
+	return tf, retFlags, nil
+}
+
+// TestFailedOpenDoesNotCloseAnotherFile opens one database successfully, then
+// fails to open a second, and checks the first database's file is still open.
+//
+// sqlite calls xClose on a file whose xOpen failed whenever pMethods is set.
+// s3vfsOpen set pMethods unconditionally, but a failed goVFSOpen never stamped
+// an id into the (zeroed) s3vfsFile — so that xClose arrived with id 0 and
+// closed fileMap[0]: the first file this process ever opened, out from under
+// the connection that owns it. That connection then failed every read that
+// missed its page cache, with errors ("SQL logic error", fts5 corruption
+// reports) that pointed anywhere but here.
+func TestFailedOpenDoesNotCloseAnotherFile(t *testing.T) {
+	vfs := &recordingVFS{TmpVFS: newTempVFS()}
+
+	vfsName := "recordingvfs"
+	if err := RegisterVFS(vfsName, vfs); err != nil {
+		t.Fatal(err)
+	}
+
+	db, err := sql.Open("sqlite3", fmt.Sprintf("first.db?vfs=%s", vfsName))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer db.Close()
+
+	if _, err := db.Exec(`CREATE TABLE t (id INTEGER PRIMARY KEY)`); err != nil {
+		t.Fatal(err)
+	}
+
+	if len(vfs.opened) == 0 {
+		t.Fatal("no file was opened for first.db")
+	}
+	first := vfs.opened[0]
+
+	// mode=ro of a missing file: TmpVFS refuses the open.
+	missing, err := sql.Open("sqlite3", fmt.Sprintf("file:does-not-exist.db?mode=ro&vfs=%s", vfsName))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := missing.Ping(); err == nil {
+		t.Fatal("opening a missing database in mode=ro should fail")
+	}
+	missing.Close()
+
+	if _, err := first.f.Stat(); err != nil {
+		t.Fatalf("first.db's file was closed by the failed open of an unrelated file: %v", err)
+	}
+
+	var count int
+	if err := db.QueryRow(`SELECT COUNT(*) FROM t`).Scan(&count); err != nil {
+		t.Fatalf("first.db is broken after the failed open of an unrelated file: %v", err)
+	}
+}

sqlite3vfs.c

@@ -61,6 +61,13 @@ int s3vfsNew(char* name, int maxPathName) {
 
 int s3vfsOpen(sqlite3_vfs* vfs, const char * name, sqlite3_file* file, int flags, int *outFlags) {
   int ret = goVFSOpen(vfs, name, file, flags, outFlags);
+  /* sqlite calls xClose on a failed open whenever pMethods is set, and a
+  ** failed goVFSOpen never registered this file -- its zeroed id would alias
+  ** another live file. NULL means there is nothing to close. */
+  if (ret != SQLITE_OK) {
+    file->pMethods = 0;
+    return ret;
+  }
   file->pMethods = &s3vfs_io_methods;
   return ret;
 }

sqlite3vfscgo.go

@@ -17,9 +17,10 @@ import (
 var (
 	vfsMap = make(map[string]ExtendedVFSv1)
 
-	fileMux    sync.Mutex
-	nextFileID uint64
-	fileMap    = make(map[uint64]File)
+	fileMux sync.Mutex
+	// Starts at 1 so a zeroed s3vfsFile's id can never name a live file.
+	nextFileID uint64 = 1
+	fileMap           = make(map[uint64]File)
 )
 
 func newVFS(name string, goVFS ExtendedVFSv1, maxPathName int) error {