Detect cleanups/v4 #9974

victorjulien · 2023-12-05T19:48:05Z

#9949 rebased

Hitting the recursion limit should be rare.

Integrate with rest of content inspect code.

Use stack local var instead of DetectEngineThreadCtx member. Make sure the limit is a const so we can avoid rereading it.

Flatten else branches after terminating ifs.

Since recursive content matching goes through the buffer from left to right, it is possible to bail early when isdataat is part of the recursive checking. If `isdataat:50,relative` fails for offset 10, it will surely also fail for offset 20. So break inspection in such cases. The exception is for dynamic isdataat, where the value is determined by a prior byte_extract that may be updated during the recursion.

DetectEngineThreadCtx::replist is managed elsewhere.

Adjust includes to enable this.

Move reference count to top of DetectEngineThreadCtx, to move it to the same cache line as the other members that are checked first in Detect().

Move members used by DetectEngineContentInspection() to the same cache line.

Core logic of the tests moved to content inspection code in separate commit.

codecov · 2023-12-05T20:31:14Z

Codecov Report

Merging #9974 (6d3bfa9) into master (c1bf955) will decrease coverage by 0.01%.
The diff coverage is 95.74%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9974      +/-   ##
==========================================
- Coverage   82.42%   82.41%   -0.01%     
==========================================
  Files         970      970              
  Lines      271363   271245     -118     
==========================================
- Hits       223667   223551     -116     
+ Misses      47696    47694       -2

Flag	Coverage Δ
fuzzcorpus	`64.45% <79.81%> (+<0.01%)`	⬆️
suricata-verify	`61.31% <85.32%> (-0.05%)`	⬇️
unittests	`62.87% <81.56%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2023-12-05T22:45:02Z

Information: QA ran without warnings.

Pipeline 16884

jasonish

In general I think some commits could use a little more description.

detect/dns.query: use new content inspect entry

Why?

detect/content-inspect: localize recursion counting

Again, perhaps a little more detail.

Given there is no ticket for these.

jufajardini

Mostly looks ok to me, have one double check comment and a question about initialization of a struct value inline.

jufajardini · 2023-12-06T15:25:45Z

src/detect-engine-content-inspection.h

@@ -28,7 +28,7 @@
 /** indication to content engine what type of data
 *  we're inspecting
 */
-enum {
+enum DetectContentInspectionType {


Any reason not to use typedef?

These days I'm finding things that are more explicit easier to work with. Typedef hides important detail.

jufajardini · 2023-12-06T15:32:27Z

src/detect-engine-content-inspection.c

+bool DetectEngineContentInspectionBuffer(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx,
+        const Signature *s, const SigMatchData *smd, Packet *p, Flow *f, const InspectionBuffer *b,
+        const enum DetectContentInspectionType inspection_mode)
+{


Do I understand correctly that the difference between this and DetectEngineContentInspection is that this one uses specific types for buffer and inspection type? If so, I wonder if this difference, or an indication of preference of usage should be added to the function description or commit.

I've update the commit msg. It's a convenience function that keeps the internals of InspectionBuffer hidden from the callers.

jufajardini · 2023-12-06T15:45:54Z

src/detect-engine-content-inspection.c

-    if (unlikely(det_ctx->inspection_recursion_counter == de_ctx->inspection_recursion_limit)) {
+    ctx->recursion.count++;
+    if (unlikely(ctx->recursion.count == ctx->recursion.limit)) {


I saw that before calling DetectEngineContentInspectionInternal from DetectEngineContentInspection, ctx->recursion.limit was set. However, for other direct calls of DetectEngineContentInspectionInternal I didn't see that being defined. For those cases, is this being defined elsewhere that I missed?

both DetectEngineContentInspectionBuffer and DetectEngineContentInspection set it

the ctx is shared between consecutive recursive calls then, it's actually meant to count and limit that recursion

jufajardini · 2023-12-06T15:56:50Z

src/detect-engine-content-inspection.c

-            endian |= (uint8_t)((flags & DETECT_CI_FLAGS_DCE_LE) ? LittleEndian : BigEndian);
+            endian = (uint8_t)((flags & DETECT_CI_FLAGS_DCE_LE) ? LittleEndian : BigEndian);


Is this connected to the rest of the changes here?

it's a minor clarification. The resulting value is the same.

jufajardini · 2023-12-06T16:02:29Z

src/tests/detect-http-uri.c

-    UtRegisterTest("UriTestSig30", UriTestSig30);
-    UtRegisterTest("UriTestSig31", UriTestSig31);


Just to double check: is this the commit that added similar logic elsewhere? 32b4423

heh, no, it's in a branch that is still WIP. I'll drop this commit from here.

victorjulien · 2023-12-06T16:11:18Z

In general I think some commits could use a little more description.

detect/dns.query: use new content inspect entry

Why?

detect/content-inspect: localize recursion counting

Again, perhaps a little more detail.

Given there is no ticket for these.

Added some explanations

victorjulien · 2023-12-07T05:35:43Z

replaced by #9986

victorjulien added 18 commits December 5, 2023 14:56

detect/content-inspect: reduce scope of variables

e81aa24

detect/content-inspect: pass const to inspect func

cc876b5

detect/content-inspect: remove const casting

fae1af6

detect/content-inspect: assist branch prediction

ea33028

Hitting the recursion limit should be rare.

detect/content-inspect: switch type of enum

b1d35b3

detect/content-inspect: add entry for InspectionBuffer

11a930c

detect/dns.query: use new content inspect entry

c1e10ab

detect/krb5.sname: use new content inspect entry

c5dbb3c

detect/base64: move content inspection logic

68a3d0a

Integrate with rest of content inspect code.

detect/content-inspect: reduce scope of internal func

8078a73

detect/content-inspect: localize recursion counting

a2428cf

Use stack local var instead of DetectEngineThreadCtx member. Make sure the limit is a const so we can avoid rereading it.

detect/content-inspect: flatten branches

890eb71

Flatten else branches after terminating ifs.

detect/payload: remove unneeded pointer reset

45400a7

DetectEngineThreadCtx::replist is managed elsewhere.

detect/bytemath: pass match ctx directly

b61923a

Adjust includes to enable this.

detect: optimize struct layout

8de4a76

Move reference count to top of DetectEngineThreadCtx, to move it to the same cache line as the other members that are checked first in Detect().

detect/content-inspect: optimize struct layout

6a3d9c3

Move members used by DetectEngineContentInspection() to the same cache line.

detect/uri: remove tests that are now done elsewhere

6d3bfa9

Core logic of the tests moved to content inspection code in separate commit.

victorjulien mentioned this pull request Dec 5, 2023

Detect cleanups/v3 #9949

Closed

jasonish approved these changes Dec 6, 2023

View reviewed changes

jufajardini reviewed Dec 6, 2023

View reviewed changes

victorjulien mentioned this pull request Dec 6, 2023

Detect cleanups/v6 #9986

Closed

victorjulien closed this Dec 7, 2023

victorjulien deleted the detect-cleanups/v4 branch February 20, 2024 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect cleanups/v4 #9974

Detect cleanups/v4 #9974

victorjulien commented Dec 5, 2023

codecov bot commented Dec 5, 2023

suricata-qa commented Dec 5, 2023

jasonish left a comment •

edited

Loading

jufajardini left a comment

jufajardini Dec 6, 2023

victorjulien Dec 6, 2023

jufajardini Dec 6, 2023

victorjulien Dec 6, 2023

jufajardini Dec 6, 2023

victorjulien Dec 6, 2023

victorjulien Dec 6, 2023

jufajardini Dec 6, 2023

victorjulien Dec 6, 2023

jufajardini Dec 6, 2023

victorjulien Dec 6, 2023

victorjulien commented Dec 6, 2023

victorjulien commented Dec 7, 2023

		endian \|= (uint8_t)((flags & DETECT_CI_FLAGS_DCE_LE) ? LittleEndian : BigEndian);
		endian = (uint8_t)((flags & DETECT_CI_FLAGS_DCE_LE) ? LittleEndian : BigEndian);

		UtRegisterTest("UriTestSig30", UriTestSig30);
		UtRegisterTest("UriTestSig31", UriTestSig31);

Detect cleanups/v4 #9974

Detect cleanups/v4 #9974

Conversation

victorjulien commented Dec 5, 2023

codecov bot commented Dec 5, 2023

Codecov Report

suricata-qa commented Dec 5, 2023

jasonish left a comment • edited Loading

Choose a reason for hiding this comment

jufajardini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien commented Dec 6, 2023

victorjulien commented Dec 7, 2023

jasonish left a comment •

edited

Loading