diff --git a/failedopen_test.go b/failedopen_test.go new file mode 100644 index 0000000..93a3729 --- /dev/null +++ b/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) + } +} diff --git a/sqlite3vfs.c b/sqlite3vfs.c index 6abd1eb..e96e91f 100644 --- a/sqlite3vfs.c +++ b/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; } diff --git a/sqlite3vfscgo.go b/sqlite3vfscgo.go index 0f5f7b9..208482f 100644 --- a/sqlite3vfscgo.go +++ b/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 {