Close SQLite connection leak on Database init failure (#27)#32
Merged
Conversation
Database::Database(const char*) leaked the sqlite3* handle on two error paths: - sqlite3_open_v2 can allocate a handle even when it returns an error (everything except OOM), and that handle was discarded unclosed before throwing. - If any CreateTableQuery::Execute() throws, the constructor exits via exception. The object was never fully constructed, so ~Database does not run and the already-open connection leaks; Initialize() never assigns instance_ in this case either, so nothing else can close it. Close db_ on both paths before the exception propagates: explicitly on the open-failure branch, and via a try/catch around table creation. Because the constructor throw prevents ~Database from running at all, there is no double-close risk. sqlite3_close(nullptr) is a documented no-op, so this is safe even when db_ never got assigned. This completes #27 - the destructor and Instance() null-check were already added in #28. Adds a regression test that fails Initialize() with a path whose parent directory doesn't exist (fails identically on POSIX/Windows via SQLITE_OPEN_CREATE not creating missing directories), and confirms Initialize() can subsequently succeed with clean, recoverable state. Verified under LeakSanitizer: reproduced a real 1400-byte leak on the pre-fix code and confirmed zero leaks (full suite, 65/65 tests) with the fix applied. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Database::Database(const char*) leaked the sqlite3* handle on two error
paths:
(everything except OOM), and that handle was discarded unclosed before
throwing.
exception. The object was never fully constructed, so ~Database does not
run and the already-open connection leaks; Initialize() never assigns
instance_ in this case either, so nothing else can close it.
Close db_ on both paths before the exception propagates: explicitly on the
open-failure branch, and via a try/catch around table creation. Because the
constructor throw prevents ~Database from running at all, there is no
double-close risk. sqlite3_close(nullptr) is a documented no-op, so this is
safe even when db_ never got assigned.
This completes #27 - the destructor and Instance() null-check were already
added in #28.
Adds a regression test that fails Initialize() with a path whose parent
directory doesn't exist (fails identically on POSIX/Windows via
SQLITE_OPEN_CREATE not creating missing directories), and confirms
Initialize() can subsequently succeed with clean, recoverable state.
Verified under LeakSanitizer: reproduced a real 1400-byte leak on the
pre-fix code and confirmed zero leaks (full suite, 65/65 tests) with the fix
applied.