Repository navigation
Harden crash paths and flatten && / || chains - #13
Merged
Merged
Conversation
A crash-risk review of the FFI layer and decoders found one real process crash and several traps or panics that surfaced badly. This fixes them and adds regression tests for each. - Reject query strings nested more than 12 groups deep before they reach tantivy's parser. The grammar recurses per group with no limit, so a deeply nested string overflowed the stack, which catch_unwind cannot turn into an error. It also backtracks exponentially (12 levels ~2 ms, 22 levels ~8 s), so the bound sits well below the stack limit. Applied on the string search, count and `.parsed` paths. - Detect duplicate field names when building the schema. tantivy panics on a repeat, which only surfaced as "internal panic". - Convert wide integers (`Int128`/`UInt128`) in `[String: Any]` documents with `exactly:` and throw on overflow instead of trapping. - Clamp MessagePack decoder capacity reservations to the bytes remaining, so a corrupt header cannot trigger a huge allocation. - Drop index and writer handles inside catch_unwind, since a panic escaping an extern "C" free function aborts the process. - Cap structured `Query` nesting at 40 levels with a clear error; the engine's JSON parser already refused deeper trees opaquely. - Flatten `&&` and `||` chains into one clause list when a side is a pure must/should boolean. Scoring is unchanged (BM25 sums clause scores either way) and long chains no longer hit the nesting limit. Mixed operators, `minimumShouldMatch` and `excluding` keep their shape. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Resolves the conflict in tantivy_writer_free: keep main's wait for background merges (#14) and run it inside catch_unwind, so a panic in the drop path cannot escape the extern "C" boundary and abort. Co-Authored-By: Claude Fable 5.1 <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.
Summary
A crash-risk review of the FFI layer and the MessagePack/JSON decoders found one genuine process crash and several traps or panics that surfaced as opaque errors. This PR fixes them, adds regression tests for each, and flattens
&&/||chains so long queries no longer hit a nesting limit.What changed
Rust (
rust/src/lib.rs)catch_unwindguard could not convert it. Query strings are now walked before parsing and rejected past 12 nested groups, on the stringsearch,countand.parsedpaths. The bound is far below the stack limit because the parser also backtracks exponentially: measured parse time was ~2 ms at 12 levels, ~45 ms at 16 and ~8 s at 22. Backslash-escaped parens are not counted.tantivy_index_freeandtantivy_writer_freedrop insidecatch_unwind. A panic escaping anextern "C"function aborts the process, and these had no error out-parameter to route throughguard.Swift
MessagePack.swift: theSignedInteger/UnsignedIntegerfallbacks in the untyped document encoder usedInt64(_:), which traps on overflow. They now useexactly:and throw an encoding error, so an out-of-rangeInt128is reported rather than aborting.MessagePack.swift: decoder capacity reservations are clamped to the bytes remaining in the buffer, so a corrupt header cannot trigger a multi-gigabyte allocation.Query.swift: structured query validation capsboost/booleannesting at 40 levels with a clear error. The engine's JSON parser already refused deeper trees with an opaque recursion-limit message.Query.swift:&&and||flatten.a && b && cis now onemustlist of three rather than a boolean nested in a boolean. Merging is exact for scoring (BM25 sums clause scores either way).(a || b) && c, booleans withminimumShouldMatch, andexcludingresults keep their structure.Reviewer notes
swift testhere ran against a freshly built host library.Querytree hundreds of thousands of levels deep still overflows when Swift releases it, becauseindirectenum deallocation recurses inside the runtime. The cap bounds this library's own recursion and gives a clear error but cannot make such a tree survivable.MAX_QUERY_NESTINGcomment are a ready reproduction.Testing
Full suite: 399 tests in 35 suites pass. New tests in
ValidationTests.swiftcover nested query strings (accepted at 12, rejected at 13 and 100,000), escaped parens, structured tree depth, duplicate field names, wide integers, and operator-chain flattening including the cases that must not merge.🤖 Generated with Claude Code