detect/byte: cache byte values in tx state#15196
detect/byte: cache byte values in tx state#15196
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 #15196 +/- ##
==========================================
- Coverage 82.71% 82.68% -0.03%
==========================================
Files 993 993
Lines 271737 271809 +72
==========================================
- Hits 224772 224751 -21
- Misses 46965 47058 +93
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
WARNING:
Pipeline = 30908 |
| * its own allocation so that values remain correct across multiple | ||
| * packets: a pointer swap would rotate ownership on every packet and | ||
| * leave each TX reading another TX's values on the second packet. */ | ||
| memcpy(det_ctx->byte_values, state->byte_values, state->byte_values_size * sizeof(uint64_t)); |
There was a problem hiding this comment.
optimization detail : I think we can do the swap instead of memcpy, if we care to swap back at the right time
There was a problem hiding this comment.
Yes -- the swap could be reintroduced but note that the amount of the data copied is bounded -- in practice, it's just a few uint64_t's.
|
Still questions about tickets and scope :
For the latter, you said
Is it a simple fixup to add ? Or a more complete one |
Test case 09 uses the rule/pcap from the issue 1412.
The fix for 7197 is more involved and better handled by a follow-on PR. |
OK thanks
But the ticket do not mention ticket 1412, is this solved by this PR of is there more to do ? |
1412 is partially solved -- for bidirectional rules, it's completely solved. For unidirectional rules, if the producing buffer is at a higher progress than the consuming buffer, the issue from 1412 still occurs. I'll make sure that the PR states this -- i'm rebasing and putting a replacement pr together. |
|
Continued in #15333 |
3 participants
Continuation of #15195
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
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=