fix: deterministic extraction; don't merge comparison conditions#1
Merged
Merged
Conversation
Two related fixes surfaced by a 100k-rule round-trip corpus:
- matchDetectionItems iterated the detection-item map without ordering, so
quantifier evaluation ("N of selection_*", "all of them") was
non-deterministic. Sort the matched names.
- groupORConditions folded ordering comparisons (>, >=, <, <=) on the same
field across an OR into a single Alternatives condition. `f > a OR f > b` is
not a membership test, so this silently dropped a bound — and, because the
merge was position-dependent, made extraction non-deterministic. Exclude
comparison operators from the merge.
Adds regression tests asserting both same-field bounds survive across 50 runs.
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.
Two related correctness bugs surfaced while running a 100,000-rule Sigma→EQL round-trip corpus. Both could silently drop a condition and made extraction non-deterministic.
Fixes
matchDetectionItemsmap-order non-determinism — quantifier evaluation (N of selection_*,all of them) iterated the detection-item map without ordering, so collected selection order (and downstream condition order) varied per run. Now sorted.groupORConditionsmerges ordering comparisons — it foldedfield > a OR field > binto oneAlternativescondition.f > a OR f > bis not a membership test, so a bound was silently dropped; the position-dependent merge made the drop non-deterministic. Ordering operators (>,>=,<,<=) are now excluded (equality/pattern operators still fold as before).Tests
determinism_test.goasserts both same-field bounds survive across 50 runs and comparisons never gain alternatives. Full existing suite passes.Found via the eql-parser Sigma↔EQL translator corpus (the same over-broad-merge bug existed there and was fixed in parallel). Consumers pin a released version, so nothing auto-updates.