Repository navigation
Guard every string crossing the FFI, not just the query - #8
Merged
Merged
Conversation
An interior NUL truncates a Swift string at the C boundary. validateQueryString
covered the query string; every other string reaching the engine was unguarded,
and truncating to a prefix that is itself a valid field name silently operates
on that field instead:
deleteDocuments(field: "title\0Slug", equals: "red") -> 2 docs -> 1 doc
deleteDocuments(field: "titleSlug", equals: "red") -> 2 docs -> 2 docs
The requested field matches nothing, yet a document is deleted. Same
substitution on search(fields:), highlight:, orderBy:, and aggregate(), where
trailing bytes after a NUL are silently dropped.
Also fixes two smaller ones found alongside it:
- Float decode overflowed to infinity. Float(1e300) is +inf, so an
out-of-range f64 decoded as an infinity while Double kept the value and
every integer conversion range-checks and throws.
- writer(heapSize:) and termCounts(limit:) accepted negatives, surfacing as
the engine's arena-cap error or an opaque serde message rather than saying
what was passed.
Adds coverage for behaviour that was already correct but untested: write's
all-or-nothing guarantee and its lock release, UInt64.max round-tripping,
sub-second dates truncating rather than rounding, the empty-boolean and
minimumShouldMatch match-nothing cases, and the three MoreLikeThisOptions
knobs no test referenced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
An interior NUL truncates a Swift string at the C boundary.
validateQueryStringalready guarded the query string against exactly this; every other string reaching the engine was unguarded. Field names did truncate, and when the truncated prefix is itself a valid field name the call silently operates on that field instead of erroring.The one with teeth
Demonstrated before fixing, on an index with
titleandtitleSlug:titleSlugvalues arered-slug/blue-slug, so the requested field matches nothing — yet a document is deleted, because the name truncated totitle. The same silent substitution applied tosearch(fields:)(returned a hit fromtitle),highlight:(highlightedtitle),orderBy:, andaggregate(), wherejson + "\0garbage"ran the parseable prefix and dropped the rest without complaint.Fixed by generalizing the existing guard into
Index.validateNoInteriorNUL/validateFieldNameand applying it at every boundary:fields,highlight,orderBy.field,boostskeys,deleteDocuments(field:),aggregate,termCounts. The comma check comes along for free — field names join into a CSV, so an embedded comma widens the field set the same way a NUL narrows it.The structured
Querypath was already safe: JSON-escaping preserves the NUL, so the engine rejects the field outright rather than substituting one.Two smaller ones found alongside
Floatdecode overflowed to infinity.Float(try Convert.double(...))at three call sites, so a stored1e300decoded asinfin the keyed, unkeyed, and single-value containers — whileDoublekept the value and every integer conversion range-checks viaexactly:and throws. Silently substituting infinity for a real measurement is the worst of the three options. AddedConvert.floatwith a range check.Missing argument validation.
writer(heapSize: -1)wrapped tousize::MAXand surfaced as the engine's per-thread arena-cap error, which never mentions the value passed;termCounts(limit: -1)surfaced as an opaque serdeexpected u32. Both now throw.encoding, assearchdoes forlimit.Tests
234 -> 256. Both regression tests were verified to fail without their fix — the delete test reports
documentCount → 1, the float testScalar(big: inf)— so they aren't vacuous.Coverage also added for behaviour that was already correct but untested:
write's documented all-or-nothing guarantee, and that the discarded writer releases the directory lock (nothing covered this; a leaked lock would break every subsequent write after one failed block)UInt64.maxround-tripping through store and term query, the exactnessFieldValuedocumentsallOf([])matches nothing,minimumShouldMatchabove the clause count matches nothingminWordLength/maxWordLength/maxDocFrequency— the only public symbols with no test reference at allLeft unfixed
Engine error messages containing a NUL are lost, degrading to a bare
tantivy: error(CString::newfails on the message inrust/src/lib.rs). That needs a Rust rebuild and release, and the guards here make it much harder to reach, so it's better folded into the next release.🤖 Generated with Claude Code