Fix vec0 parser token-type guards: && should be || - #319
Open
stumpylog wants to merge 1 commit into
Open
Conversation
Every "next token must be X" guard in the vec0 column/option-definition
parser (vec0_parse_table_option, vec0_parse_partition_key_definition,
vec0_parse_auxiliary_column_definition, vec0_parse_primary_key_definition,
vec0_parse_vector_column) is written as:
rc = vec0_scanner_next(&scanner, &token);
if (rc != VEC0_TOKEN_RESULT_SOME && token.token_type != TOKEN_TYPE_X) {
return SQLITE_EMPTY; // or SQLITE_ERROR
}
This is backwards in two ways:
- When a token IS present (the common case), `rc != VEC0_TOKEN_RESULT_SOME`
is false, and `false && anything` is unconditionally false -- the
token_type check never runs at all. A present-but-wrong-type token (e.g.
a digit where an identifier or '=' was expected) is silently accepted
and its (wrong) start/end pointers get used as if they were correct.
- When no token is present (EOF), `token` is left uninitialised by the
scanner. `true && token.token_type != TOKEN_TYPE_X` then reads that
uninitialised value, so whether the guard fires is undefined -- and on
the branch where it doesn't, the code falls through and dereferences
uninitialised `token.start`/`token.end`. This is what the nightly
valgrind memcheck job (not ASan/UBSan, which don't catch uninitialised
reads) reports as ~20 "use of uninitialised value" errors in
vec0_parse_* and the sqlite3_strnicmp calls they feed.
Every one of these guards should be `||`: bail if there's no token, OR
the token that is there is the wrong type. Several sibling checks in the
same functions already use `||` correctly, which is how the drift is
visible -- this makes all 20 sites consistent with those.
Fix: mechanical && -> || flip at all 20 sites. No call site needed the
buggy behavior; each one's job is purely "reject if this isn't token X".
Testing: added test_vec0_column_option_malformed_value to
tests/test-loadable.py, asserting that a distance_metric option with a
missing value (EOF right after '=') now deterministically raises rather
than falling through with an uninitialised token.
stumpylog
changed the title
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
1 participant
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.
Summary
Every "next token must be X" guard in the vec0 column/option parser uses
&&where it needs||:rc != VEC0_TOKEN_RESULT_SOMEisfalse, sofalse && Xis alwaysfalse— the type check never runs, and a wrong-type token is silently accepted.tokenis uninitialised, so the guard is undefined behavior. Nightly valgrind reports ~20 "use of uninitialised value" errors here; ASan/UBSan don't catch uninitialised reads.20 sites have this pattern, across
vec0_parse_table_option,vec0_parse_partition_key_definition,vec0_parse_auxiliary_column_definition,vec0_parse_primary_key_definition, andvec0_parse_vector_column. Sibling checks in the same functions already correctly use||.Fix
Mechanical
&&->||at all 20 sites.Testing
Added
test_vec0_column_option_malformed_value: adistance_metricoption with a missing value now deterministically raises instead of falling through with an uninitialised token.🤖 Generated with Claude Code