Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lua xform/v21 #12425

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Lua xform/v21 #12425

wants to merge 9 commits into from

Conversation

victorjulien
Copy link
Member

#12251, with following changes/updates:

  • fix error handling
  • don't supply size/len as separate args to the script
  • remove init logic, as it was unused
  • make sure Packet and Flow are available
  • rebased to master

SV_BRANCH=OISF/suricata-verify#2240

jlucovsky and others added 9 commits January 18, 2025 12:02
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.
Adds a new lua script capability to use a script as a buffer transform
keyword.

It uses a `transform` lua function that returns the input buffer after
modifying it.

Issue: 2290
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
So transforms can access them through DetectEngineThreadCtx
Copy link

codecov bot commented Jan 18, 2025

Codecov Report

Attention: Patch coverage is 88.27586% with 51 lines in your changes missing coverage. Please review.

Project coverage is 80.62%. Comparing base (8f6795d) to head (31b1e19).
Report is 56 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12425      +/-   ##
==========================================
- Coverage   80.63%   80.62%   -0.01%     
==========================================
  Files         918      919       +1     
  Lines      258696   258900     +204     
==========================================
+ Hits       208598   208746     +148     
- Misses      50098    50154      +56     
Flag Coverage Δ
fuzzcorpus 56.75% <48.93%> (-0.06%) ⬇️
livemode 19.41% <13.23%> (+<0.01%) ⬆️
pcap 44.24% <31.44%> (-0.07%) ⬇️
suricata-verify 63.27% <85.81%> (+0.04%) ⬆️
unittests 58.49% <27.12%> (-0.03%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24263

int count = 0;
char *saveptr = NULL;
char *token = strtok_r(lua->copystr, ",", &saveptr);
while (token != NULL && count < LUAXFORM_MAX_ARGS) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlucovsky is there a point in having a list of predefined keys that have a specific meaning, like bytes and offset in some of your examples? We'll have to decide now to avoid collisions with scripts that may be in use in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victorjulien Although the examples show bytes and offsets, there are no predefined options. The current logic accepts up to 10 comma-separated values and passes those in args to the script.

@@ -984,7 +986,7 @@ static void DetectBufferTypeFreeFunc(void *data)
static int DetectBufferTypeInit(void)
{
BUG_ON(g_buffer_type_hash);
g_buffer_type_hash = HashListTableInit(256, DetectBufferTypeHashNameFunc,
g_buffer_type_hash = HashListTableInitWithCtx(256, DetectBufferTypeHashNameFunc,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this never freed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is. I checked master and I'm not seeing the free happen by the time exit is called

 $ git diff
diff --git a/src/util-hashlist.c b/src/util-hashlist.c
index 085a988afe..0b340ea606 100644
--- a/src/util-hashlist.c
+++ b/src/util-hashlist.c
@@ -108,7 +108,9 @@ void HashListTableFree(HashListTable *ht)
     if (ht->array != NULL)
         SCFree(ht->array);

-    SCFree(ht);
+    void *ptr = ht;
+    memset(ht, 0, sizeof(*ht));
+    SCFree(ptr);
 }

 int HashListTableAdd(HashListTable *ht, void *data, uint16_t datalen)

And via gdb

Thread 1 "Suricata-Main" hit Breakpoint 1, __GI_exit (status=status@entry=0) at ./stdlib/exit.c:137
warning: 137	./stdlib/exit.c: No such file or directory
(gdb) p *g_buffer_type_hash
$1 = {array = 0x55555636a240, listhead = 0x555556295de0, listtail = 0x5555563e8680, array_size = 256, Hash = 0x5555557b3680 <DetectBufferTypeHashNameFunc>,
 Compare = 0x5555557b3700 <DetectBufferTypeCompareNameFunc>, Free = 0x5555557b3770 <DetectBufferTypeFreeFunc>}

If it was freed, the dump from gdb should've shown 0's.

Am I missing something?

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the status as this PR needs at least a rebase (and a free avoid a leak)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

4 participants