Repository navigation
sqld: fix copy-pasted check in legacy HTTP savepoint guard - #2303
Open
freedom-winds wants to merge 1 commit into
Open
freedom-winds wants to merge 1 commit into
freedom-winds wants to merge 1 commit into
Conversation
parse_queries() tested is_release() twice, so SAVEPOINT statements slipped through the guard that is supposed to forbid them: the batch reported success while the write stayed uncommitted inside a never-released savepoint and was rolled back when the connection closed at the end of the request. Check is_savepoint() as well, and cover the guard with unit tests.
This branch has not been deployed
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.
The guard in
parse_queriesis supposed to forbid savepoint statements in the legacy HTTP API ("It's too complicated to predict the state of a transaction with savepoints"), but the predicate checkedis_release()twice, soSAVEPOINTstatements slipped through.I noticed this while poking the legacy HTTP API with transaction-ish statements:
{"statements": ["SAVEPOINT sp", "INSERT INTO t VALUES (1)"]}responds with both statements succeeding, yet nothing is committed — the write lives in a never-released savepoint, and since each request gets a fresh connection that is dropped afterwards, SQLite rolls the transaction back. The client is told the row was persisted when it wasn't. (RELEASE/ROLLBACK TOare correctly rejected, so the same batch cannot make the write durable either.)This fixes the second term to
is_savepoint()and adds unit tests for the guard.