gen/refactor: transform and misc related changes for improved transform support #12502

jlucovsky · 2025-01-29T14:34:37Z

Continuation of #12427

A set of changes from #12425 that improve transform support

Describe changes:

Detection engine thread context pointer now available to individual transform functions
Detect engine context available to transform free function
Defer memory release of keyword hash table until engine context has been reed
Make packet/tx_id available earlier
Refactor transform setup/apply pattern from 2 to 1 function: InspectionBufferSetupAndApplyTransforms

Updates:

Restored g_buffer_type_hash to not rely on the de_ctx as there is not a dependency for this global data structure.

Provide values to any of the below to override the defaults.

To use an LibHTP, Suricata-Verify or Suricata-Update pull request,
link to the pull request in the respective _BRANCH variable.
Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

This commit makes the detection engine thread context available for transforms to use. The Lua transform requires this value. Issue: 2290

Issue: 2290 This commit extends the hash table logic with an alternate free function that provides the detection engine context. Users that wish to use the next functionality must use the HashListTableInitWithCtx function when initializing the hash table. Using this interface will result in the hash table "free with context" function (new) being used instead.

Issue: 2290 Defer freeing the keyword hash table until the engine context has been freed. This eliminates a double-free from occurring. For the unittests ONLY, clear the keyword_hash to prevent a double free attempt.

So transforms can access them through DetectEngineThreadCtx

git grep -A 1 -w InspectionBufferSetup shows numbers cases of the pattern: - InspectionBufferSetup - InspectionBufferApplyTransforms Refactor the implementations of those functions into InspectionBufferSetupAndApplyTransforms to reduce function call count. Issuer: 2290

codecov · 2025-01-29T14:59:30Z

Codecov Report

Attention: Patch coverage is 92.07547% with 21 lines in your changes missing coverage. Please review.

Project coverage is 80.56%. Comparing base (cfbf8fd) to head (1161082).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #12502   +/-   ##
=======================================
  Coverage   80.56%   80.56%           
=======================================
  Files         925      925           
  Lines      259292   259322   +30     
=======================================
+ Hits       208906   208935   +29     
- Misses      50386    50387    +1

Flag	Coverage Δ
fuzzcorpus	`56.14% <79.05%> (-0.01%)`	⬇️
livemode	`19.41% <18.57%> (+0.01%)`	⬆️
pcap	`44.22% <49.01%> (+0.02%)`	⬆️
suricata-verify	`63.39% <88.14%> (+<0.01%)`	⬆️
unittests	`58.45% <41.13%> (+<0.01%)`	⬆️

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

suricata-qa · 2025-01-29T17:05:03Z

Information: QA ran without warnings.

Pipeline 24425

catenacyber · 2025-01-29T20:34:44Z

src/detect.h

@@ -1291,7 +1291,7 @@ typedef struct SigTableElmt_ {
        uint8_t flags, File *, const Signature *, const SigMatchCtx *);

    /** InspectionBuffer transformation callback */
-    void (*Transform)(InspectionBuffer *, void *context);
+    void (*Transform)(DetectEngineThreadCtx *, InspectionBuffer *, void *context);


This requires to bump up SC_PLUGIN_API_VERSION

This requires to bump up SC_PLUGIN_API_VERSION

Why? Its currently at 8, and should stay that way until 8 is released.

It should be modified every time the API gets modified so that runtime suricata and plugins can ensure compatibility

Maybe the current 8 value is not the best fit since 8 is not released...

Our compatibility promises are for a major version. So 8 is fine, as 8 is not released there is no promise that its stable until 8 is released.

There is no promise, but we can do it

We'll need to work on the versioning, so I think its out of scope for this PR.

For example, if we ever did an 8.1 release, we wouldn't consider that compatible with 8.0. So we'd want something like:

800000 # 8.0

800100 # 8.1
Or were you thinking just an ever increasing int?

Patch releases should not be considered as they should not break the API. I'd still not consider pre-release git snapshot versions either.

As the library matures we'll end up in a position where any change to any public function signature will require an update to this, which could be a lot of commits during a major development cycle.

I think you forget to take into account the people using git master (like after 8 release before 9)

We'll need to work on the versioning

indeed

so I think its out of scope for this PR.

But a change in SC_PLUGIN_API_VERSION is required by this PR

As the library matures we'll end up in a position where any change to any public function signature will require an update to this, which could be a lot of commits during a major development cycle.

I think we are there

In git master we make no promises until it is released as a final 8.0.0 release. Ppl relying on a dev branch will have to accept we can still break things.

I understand that.

Even bumping SC_PLUGIN_API_VERSION, this PR still "breaks things", but it will give a better error message instead of a segfault.

This is to get the good habit for post 8 release

catenacyber · 2025-01-29T20:37:00Z

src/detect-engine.c

@@ -977,12 +978,18 @@ static void DetectBufferTypeFreeFunc(void *data)
                    sigmatch_table[map->transforms.transforms[i].transform].name);
            continue;
        }
-        sigmatch_table[map->transforms.transforms[i].transform].Free(NULL, map->transforms.transforms[i].options);
+        sigmatch_table[map->transforms.transforms[i].transform].Free(
+                de_ctx, map->transforms.transforms[i].options);


Looks ok but feels painful

Why will we need this de_ctx when freeing ?

The Luaxform will need the de_ctx to unregister the thread ctx (see #12425 detect-transform-luaxform.c, function DetectTransformLuaxformFree)

What is this thread ctx ?

Another option would be to have sigmatch_table having 2 different function pointers Free and
FreeCtx (not sure it is better, but requires less modifications)

catenacyber · 2025-01-29T20:37:33Z

src/detect-engine.c

@@ -2606,6 +2606,9 @@ DetectEngineCtx *DetectEngineCtxInitWithPrefix(const char *prefix, uint32_t tena
 static void DetectEngineCtxFreeThreadKeywordData(DetectEngineCtx *de_ctx)
 {
    HashListTableFree(de_ctx->keyword_hash);
+#if UNITTESTS
+    de_ctx->keyword_hash = NULL;


Double free Happens only in unit tests, right ? Just double checking

And why was CI green if there was this double free ?

Yes, this is a unit test only situation.

The double free would occur in DetectUnregsiterThreadCtxFuncs iff keyword hash wasn't nullified.

And why was CI green if there was this double free ?

catenacyber · 2025-01-29T20:38:48Z

src/detect-engine-mpm.h

@@ -113,7 +113,6 @@ void DetectEngineFrameMpmRegister(DetectEngineCtx *de_ctx, const char *name, int
                const DetectBufferMpmRegistry *mpm_reg, int list_id),
        AppProto alproto, uint8_t type);

-


Does not deserve its own commit :-p

catenacyber · 2025-01-29T20:40:23Z

src/detect-dce-stub-data.c

-        InspectionBufferSetup(det_ctx, list_id, buffer, data, data_len);
-        InspectionBufferApplyTransforms(det_ctx, buffer, transforms);
+        InspectionBufferSetupAndApplyTransforms(
+                det_ctx, list_id, buffer, data, data_len, transforms);


Can you use DetectHelperGetData to save even more lines ?

Yes -- I think that pattern could be used in several places but is probably best done in a separate PR, imo

So, why do you need this commit ? if it "is probably best done in a separate PR"

catenacyber

Needs the SC_PLUGIN_API_VERSION bump

jlucovsky and others added 6 commits January 29, 2025 09:18

transforms: Make ctx available

5cf7ba4

This commit makes the detection engine thread context available for transforms to use. The Lua transform requires this value. Issue: 2290

detect/engine: Prevent double-free of keyword hash

1de19ed

Issue: 2290 Defer freeing the keyword hash table until the engine context has been freed. This eliminates a double-free from occurring. For the unittests ONLY, clear the keyword_hash to prevent a double free attempt.

detect: make packet and tx_id available earlier

cfb7d18

So transforms can access them through DetectEngineThreadCtx

clang/format: Formatting fixups

a03b777

jlucovsky requested review from jasonish and victorjulien as code owners January 29, 2025 14:34

jlucovsky mentioned this pull request Jan 29, 2025

gen/refactor: transform and misc related changes for improved transform support #12427

Closed

catenacyber reviewed Jan 29, 2025

View reviewed changes

catenacyber requested changes Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gen/refactor: transform and misc related changes for improved transform support #12502

gen/refactor: transform and misc related changes for improved transform support #12502

jlucovsky commented Jan 29, 2025

codecov bot commented Jan 29, 2025

suricata-qa commented Jan 29, 2025

catenacyber Jan 29, 2025

jasonish Jan 29, 2025

catenacyber Jan 29, 2025

jasonish Jan 29, 2025

catenacyber Jan 29, 2025

jasonish Jan 29, 2025 •

edited

Loading

catenacyber Jan 30, 2025

victorjulien Jan 30, 2025

catenacyber Jan 30, 2025

catenacyber Jan 30, 2025

catenacyber Jan 29, 2025

jlucovsky Jan 30, 2025

catenacyber Jan 30, 2025

catenacyber Jan 30, 2025

catenacyber Jan 29, 2025

catenacyber Jan 29, 2025

jlucovsky Jan 30, 2025

catenacyber Jan 30, 2025

catenacyber Jan 29, 2025

catenacyber Jan 29, 2025

jlucovsky Jan 30, 2025

catenacyber Jan 30, 2025

catenacyber Jan 30, 2025

catenacyber left a comment

		@@ -113,7 +113,6 @@ void DetectEngineFrameMpmRegister(DetectEngineCtx de_ctx, const char name, int
		const DetectBufferMpmRegistry *mpm_reg, int list_id),
		AppProto alproto, uint8_t type);

gen/refactor: transform and misc related changes for improved transform support #12502

Are you sure you want to change the base?

gen/refactor: transform and misc related changes for improved transform support #12502

Conversation

jlucovsky commented Jan 29, 2025

Provide values to any of the below to override the defaults.

codecov bot commented Jan 29, 2025

Codecov Report

suricata-qa commented Jan 29, 2025

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

jasonish Jan 29, 2025 • edited Loading

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

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

catenacyber left a comment

Choose a reason for hiding this comment

jasonish Jan 29, 2025 •

edited

Loading