Repository navigation
Return an error instead of crashing when a connection is used after Close - #9
Merged
Merged
Conversation
…lose SQLite dereferences the connection handle without checking it, so every *SQLiteConn method that passes c.db into the C API crashed the process once Close had released the handle and set it to nil. Ordinary code reaches this: a connection captured in a ConnectHook outlives its *sql.DB. The affected methods now check the handle first with dbConnOpen, which reads it under c.mu and does not hold c.mu across the C call. Methods that return an error report errConnClosed, the hook and authorizer registrations, which have no error to report, do nothing, and AutoCommit, GetFilename, GetLimit and SetLimit return a documented value. Affected: RegisterCollation, RegisterCommitHook, RegisterRollbackHook, RegisterUpdateHook, RegisterAuthorizer, RegisterFunc, RegisterAggregator, AutoCommit, GetFilename, GetLimit, SetLimit, SetFileControlInt and SetFileControlInt64. This is go-sqlite3 commit a5779f8 (mattn#1462) with its test. Upstream applies it after 068ac77, which rewrites the body of RegisterAuthorizer; here the check is added to the current body.
The guards of the previous commit call dbConnOpen, which takes c.mu. Close holds c.mu while sqlite3_close_v2 runs, and SQLite runs callbacks from there: closing a connection with an open transaction rolls it back and calls the rollback hook, and with sqlite_trace the trace callback gets a close event. A callback that calls GetFilename, GetLimit or another guarded method then waits for c.mu forever and Close never returns; before the guards the call worked. The methods already read c.db without c.mu to pass it to SQLite, so they now check it the same way: they still report a closed connection, and take no lock. AutoCommit took c.mu before the guards and is unchanged. go-sqlite3 has the same deadlock since a5779f8.
With sqlite_app_armor, which the full tag set of the workflow includes, SQLite refuses calls on a connection it is closing, so GetFilename returned an empty name in the rollback hook and TestConnMethodsDuringClose failed. The test is about Close returning; it no longer checks what the method returns.
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
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.
Calling connection methods after
Closecan pass a nil handle to SQLite and crash the process. Return a closed-connection error, ignore callback registrations, or return the documented sentinel value instead.Backports mattn#1462 (a5779f8), with one adjustment: the new guards inspect the handle without acquiring
c.mu.Closeholds that mutex while SQLite runs rollback and close callbacks, so acquiring it again from a callback would deadlock. Concurrent method calls andCloseremain unsupported; the existingAutoCommitcallback deadlock is outside this change.Merged current
develop(b38e872) and resolved theRegisterAuthorizerconflict by keeping both the closed-handle guard and #6's callback-handle ownership logic. The final diff changes onlysqlite3.goandsqlite3_test.go. Squash merge is appropriate because an intermediate commit's test was corrected later.Validation of head 2f10790 on Linux amd64, Go 1.27.3:
sqlite_trace sqlite_app_armor.libsqlite3, the workflow's full and vacuum tags, and CovenantSQL's regular and miner tags.testing.T.Fataldiagnostics insqlite3_opt_unlock_notify_test.goreproduce identically on basedevelopb38e872.Earlier validation details remain available in the PR edit history. Open-connection behavior is unchanged; this PR does not claim safety when another goroutine closes the same connection concurrently.