detect/byte: cache byte values in tx state#15333
Open
detect/byte: cache byte values in tx state#15333
Conversation
det_ctx->byte_values is shared across every transaction a worker touches. Once an inspect engine finishes for a signature, its ID is recorded in inspect_flags and it won't run again when the rule resumes on a later packet. That means a value produced by byte_extract or byte_math can get clobbered by another transaction before the consumer buffer ever runs. Store produced values in a byte_values array on the per-TX DetectEngineState to fix this: - Save to the TX cache after byte_extract or byte_math succeeds. - Copy (memcpy) into det_ctx->byte_values at the start of each content inspection pass (recursion.count == 0). - Free the TX cache on detect engine reload -- local IDs can change with a new ruleset, so the next save reallocates at the right size. - Free on state destruction. Document the cache in devguide/transactions.rst. Issue: 7801
byte_extract and byte_math values can now be used in a different buffer or direction than where they were produced, as long as the producing buffer has a lower progress value than the consuming buffer -- progress order is what determines which buffer gets inspected first. - differences-from-snort: drop the paragraph noting that cross-buffer byte variable usage was unsupported - payload-keywords: add cross-buffer notes and an example to the byte_extract and byte_math sections Issue: 7801
jlucovsky
requested review from
jasonish,
jufajardini and
victorjulien
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15333 +/- ##
==========================================
- Coverage 82.64% 82.64% -0.01%
==========================================
Files 995 995
Lines 271075 271131 +56
==========================================
+ Hits 224042 224070 +28
- Misses 47033 47061 +28
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
WARNING:
Pipeline = 31239 |
|
that qa run was bad. rerunning. |
|
Information: QA ran without warnings. Pipeline = 31244 |
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
3 participants
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.
Continuation of #15196
Add per-transaction byte value caching so that variables produced by byte_extract and byte_math persist across buffer and direction boundaries.
Without this change, bidirectional (=>) rules that produce a byte variable in a toserver buffer and consume it in a toclient buffer fail when HTTP/1.1 pipelining creates multiple transactions. All TXs share the same det_ctx->byte_values, and a later TX's toserver inspection clobbers values before an earlier TX's toclient inspection runs.
The fix saves byte values to the per-TX DetectEngineState after extraction, restores them before each content inspection pass, clears them when resetting a TX, and frees the memory when DetectEngineState is destroyed, the transaction is reset, or the DetectEngineThreadCtx is uninitialized.
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7801
Note that the issue described in Redmine ticket 1412 is partially addressed with this work when using bi-directional rules and cross-buffer variables. However, unidirectional rules that have cross-buffer support where the producer has a higher progress value than the consumer will still experience the ordering issue from 1412.
Describe changes:
Updates:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_REPO=
SV_BRANCH=OISF/suricata-verify#2968
SU_REPO=
SU_BRANCH=