From 4206e1ff6d6358157ff4696c90481b663540d45e Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Mon, 6 Jul 2020 16:42:18 -0400 Subject: [PATCH] Sqlite open bugs (#603) * Docs say: call sqlite3_close even when sqlite3_open returns error * Be careful of possible failures of AutoSave... ... In ProjectHistory operations, do the AutoSave first and throw if there is a failure. Only then change UndoManager's state, confident that it remains consistent with what was AutoSaved. * Throw exceptions if lazy opening of project's database fails... ... because the calls to DB() as in Sqlite3SampleBlock may be in deeply nested places that can't propagate the error codes; and besides, those functions had been assuming non-null returns from DB(), which might have crashed before, but now the assumption is correct when the throw didn't happen. Note that this exception may also happen during attempted Autosave. Uses of Autosave were reviewed and some changes made in the previous commit. --- src/ProjectFileIO.cpp | 16 +++++++++++++--- src/ProjectFileIO.h | 4 ++++ src/ProjectHistory.cpp | 26 ++++++++++++++++++++------ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/ProjectFileIO.cpp b/src/ProjectFileIO.cpp index 1fe476acb..d732fc648 100644 --- a/src/ProjectFileIO.cpp +++ b/src/ProjectFileIO.cpp @@ -235,12 +235,15 @@ ProjectFileIO::~ProjectFileIO() sqlite3 *ProjectFileIO::DB() { - if (mDB) + if (!mDB) { - return mDB; + OpenDB(); + if (!mDB) + throw SimpleMessageBoxException{ + XO("Failed to open the project's database") }; } - return OpenDB(); + return mDB; } sqlite3 *ProjectFileIO::OpenDB(FilePath fileName) @@ -266,6 +269,9 @@ sqlite3 *ProjectFileIO::OpenDB(FilePath fileName) if (rc != SQLITE_OK) { SetDBError(XO("Failed to open project file")); + // sqlite3 docs say you should close anyway to avoid leaks + sqlite3_close( mDB ); + mDB = nullptr; return nullptr; } @@ -647,6 +653,8 @@ bool ProjectFileIO::CopyTo(const FilePath &destpath) } else { + // sqlite3 docs say you should close anyway to avoid leaks + sqlite3_close( destdb ); SetDBError( XO("Unable to open the destination project file:\n\n%s").Format(destpath) ); @@ -1272,6 +1280,8 @@ bool ProjectFileIO::SaveCopy(const FilePath& fileName) rc = sqlite3_open(fileName, &db); if (rc != SQLITE_OK) { + // sqlite3 docs say you should close anyway to avoid leaks + sqlite3_close( db ); SetDBError(XO("Failed to open backup file")); return false; } diff --git a/src/ProjectFileIO.h b/src/ProjectFileIO.h index c34ffc7de..b1802e84d 100644 --- a/src/ProjectFileIO.h +++ b/src/ProjectFileIO.h @@ -109,7 +109,11 @@ private: static int ExecCallback(void *data, int cols, char **vals, char **names); int Exec(const char *query, ExecCB callback, wxString *result); + // The opening of the database may be delayed until demanded. + // Returns a non-null pointer to an open database, or throws an exception + // if opening fails. sqlite3 *DB(); + sqlite3 *OpenDB(FilePath fileName = {}); bool CloseDB(); bool DeleteDB(); diff --git a/src/ProjectHistory.cpp b/src/ProjectHistory.cpp index 420774efb..76e840090 100644 --- a/src/ProjectHistory.cpp +++ b/src/ProjectHistory.cpp @@ -75,6 +75,15 @@ bool ProjectHistory::RedoAvailable() const !tracks.HasPendingTracks(); } +namespace { + void AutoSaveOrThrow( ProjectFileIO &projectFileIO ) + { + if ( !projectFileIO.AutoSave() ) + throw SimpleMessageBoxException{ + XO("Automatic database backup failed.") }; + } +} + void ProjectHistory::PushState( const TranslatableString &desc, const TranslatableString &shortDesc) { @@ -87,6 +96,10 @@ void ProjectHistory::PushState(const TranslatableString &desc, { auto &project = mProject; auto &projectFileIO = ProjectFileIO::Get( project ); + if((flags & UndoPush::AUTOSAVE) != UndoPush::MINIMAL) + AutoSaveOrThrow( projectFileIO ); + + // remaining no-fail operations "commit" the changes of undo manager state auto &tracks = TrackList::Get( project ); auto &viewInfo = ViewInfo::Get( project ); auto &undoManager = UndoManager::Get( project ); @@ -96,9 +109,6 @@ void ProjectHistory::PushState(const TranslatableString &desc, desc, shortDesc, flags); mDirty = true; - - if((flags & UndoPush::AUTOSAVE) != UndoPush::MINIMAL) - projectFileIO.AutoSave(); } void ProjectHistory::RollbackState() @@ -112,14 +122,16 @@ void ProjectHistory::ModifyState(bool bWantsAutoSave) { auto &project = mProject; auto &projectFileIO = ProjectFileIO::Get( project ); + if (bWantsAutoSave) + AutoSaveOrThrow( projectFileIO ); + + // remaining no-fail operations "commit" the changes of undo manager state auto &tracks = TrackList::Get( project ); auto &viewInfo = ViewInfo::Get( project ); auto &undoManager = UndoManager::Get( project ); auto &tags = Tags::Get( project ); undoManager.ModifyState( &tracks, viewInfo.selectedRegion, tags.shared_from_this()); - if (bWantsAutoSave) - projectFileIO.AutoSave(); } // LL: Is there a memory leak here as "l" and "t" are not deleted??? @@ -129,6 +141,9 @@ void ProjectHistory::PopState(const UndoState &state) { auto &project = mProject; auto &projectFileIO = ProjectFileIO::Get( project ); + AutoSaveOrThrow( projectFileIO ); + + // remaining no-fail operations "commit" the changes of undo manager state auto &dstTracks = TrackList::Get( project ); auto &viewInfo = ViewInfo::Get( project ); @@ -146,7 +161,6 @@ void ProjectHistory::PopState(const UndoState &state) dstTracks.Add(t->Duplicate()); } - projectFileIO.AutoSave(); } void ProjectHistory::SetStateTo(unsigned int n)