diff --git a/src/database.cc b/src/database.cc index a5feea7..1e8387e 100644 --- a/src/database.cc +++ b/src/database.cc @@ -82,14 +82,26 @@ Database::Database(const char* path) : db_(nullptr) { // multiple threads; access is additionally serialized through db_mutex_. const int flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX; if (sqlite3_open_v2(path, &db_, flags, nullptr)) { + // sqlite3_open_v2 can allocate a handle even on failure (except on OOM); it must + // still be passed to sqlite3_close, which is a documented no-op if db_ is null + sqlite3_close(db_); + db_ = nullptr; throw std::invalid_argument("Database could not be initialized"); } - auto& reg = GetReflectionRegister(); - for (const auto& contents : reg.records) { - const auto& record = contents.second; - CreateTableQuery query(db_, record); - query.Execute(); + try { + auto& reg = GetReflectionRegister(); + for (const auto& contents : reg.records) { + const auto& record = contents.second; + CreateTableQuery query(db_, record); + query.Execute(); + } + } catch (...) { + // The constructor never completes on this path, so ~Database will not run to close + // db_; close it here before the exception propagates + sqlite3_close(db_); + db_ = nullptr; + throw; } } diff --git a/tests/concurrency_test.cc b/tests/concurrency_test.cc index 9fc9a02..e51df98 100644 --- a/tests/concurrency_test.cc +++ b/tests/concurrency_test.cc @@ -20,8 +20,6 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. -#include "database.h" - #include #include @@ -30,6 +28,7 @@ #include #include +#include "database.h" #include "person.h" using namespace sqlite_reflection; @@ -179,6 +178,27 @@ TEST_F(ConcurrencyTest, ReinitializeWhileAHandleIsHeldIsRejected) { EXPECT_EQ(0, static_cast(fresh->FetchAll().size())); } +TEST_F(ConcurrencyTest, InitializeLeavesCleanStateAfterOpenFailure) { + // A path inside a non-existent parent directory fails to open: SQLITE_OPEN_CREATE creates + // the leaf database file but not missing parent directories, so sqlite3_open_v2 fails + // identically on POSIX and Windows. Before the fix, the sqlite3* handle it can still + // allocate on failure leaked, since it was discarded without a matching sqlite3_close. + Database::Finalize(); + + EXPECT_THROW(Database::Initialize("nonexistent_dir_xyz/inner/db.sqlite"), std::invalid_argument); + + // The failed init must leave clean, recoverable state: instance_ was never assigned (the + // exception propagates out of the Database constructor before the owning shared_ptr is + // formed) and retired_ is untouched, so a subsequent Initialize() with a valid path must + // succeed and be fully usable. + Database::Initialize(""); + const auto db = Database::Instance(); + Person p{L"ada", L"lovelace", 36}; + db->SaveAutoIncrement(p); + const auto all = db->FetchAll(); + EXPECT_EQ(1, static_cast(all.size())); +} + TEST_F(ConcurrencyTest, RepeatedFinalizeDoesNotClearTheReinitializationGuard) { // Calling Finalize() more than once while a handle to the first database is still held // must not drop the guard: the second Finalize() (with instance_ already null) must leave