unittests: convert detect-base64-data.c to FAIL/PASS API #14058

jhonny-rs · 2025-10-17T00:20:13Z

Title: Convert base64_data unit tests to FAIL/PASS API

Description

This PR refactors the unit tests in detect-base64-data.c to strictly adhere to the Suricata unit testing API (util-unittest.h).

The functions DetectBase64DataSetupTest01 and DetectBase64DataSetupTest04 were updated to replace internal C failure logic (if (cond) { printf(...); goto end; }) with immediate assertions using the FAIL/PASS macros.

Issue informartion
Issue 6320: https://redmine.openinfosecfoundation.org/issues/6320

Specific Changes:

All subsequent assertion checks are handled by FAIL_IF_NULL() or FAIL_IF_NOT().
The cleanup logic is now unconditional, as it is only reached on the successful path (PASS).

Note on Memory Safety Policy:
Following the project's guidance that memory leaks are acceptable on unit test failure paths (as the process terminates immediately), I removed the resource-safe goto end pattern. This ensures that the code fully uses the immediate exit behavior of the FAIL/PASS API.

Testing

Verified by running the filtered unit tests:

./src/suricata -u --unittest-filter=base64

github-actions · 2025-10-17T00:51:37Z

NOTE: This PR may contain new authors.

codecov · 2025-10-17T01:31:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.44%. Comparing base (93c0409) to head (87d8d7d).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #14058   +/-   ##
=======================================
  Coverage   84.43%   84.44%           
=======================================
  Files        1011     1011           
  Lines      272251   272254    +3     
=======================================
+ Hits       229876   229904   +28     
+ Misses      42375    42350   -25

Flag	Coverage Δ
fuzzcorpus	`63.40% <100.00%> (-0.01%)`	⬇️
livemode	`19.45% <0.00%> (+0.10%)`	⬆️
pcap	`44.80% <100.00%> (-0.01%)`	⬇️
suricata-verify	`65.20% <100.00%> (+0.01%)`	⬆️
unittests	`59.44% <100.00%> (-0.01%)`	⬇️

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

inashivb · 2025-10-17T05:17:51Z

Following the project's guidance that memory leaks are acceptable on unit test failure paths (as the process terminates immediately)

~~Just to let you know, it is no longer acceptable and will cause a failure in the CI.~~ I was wrong. Please read Victor's response below.

victorjulien · 2025-10-17T05:27:33Z

Following the project's guidance that memory leaks are acceptable on unit test failure paths (as the process terminates immediately)

Just to let you know, it is no longer acceptable and will cause a failure in the CI.

Memory leaks in the failure path are acceptable, just not in the pass-path.

victorjulien · 2025-10-17T05:30:18Z

src/detect-base64-data.c

+    FAIL_IF_NOT(sm->type == DETECT_BASE64_DECODE);
+    FAIL_IF_NULL(de_ctx->sig_list->init_data->smlists[DETECT_SM_LIST_BASE64_DATA]);
+
+    SigGroupCleanup(de_ctx);


while doing these conversions a few other cleanups can be done:

declare variables on first use

remove calls to SigGroupCleanup and SigCleanSignatures (redundant as DetectEngineCtxFree handles this)

replace SigInit with DetectEngineAppendSig

Could you update these tests to incorporate this?

Sure, I’ll get to it immediately.

inashivb

Thank you for your contribution! The changes are looking OK but we need to work on the commit message. Please make sure to read the contribution guidelines https://docs.suricata.io/en/suricata-8.0.1/devguide/contributing/code-submission-process.html#commits
You could also check out previous contributions around the same issue: 4a98c4bde for examples. 😉

inashivb · 2025-10-17T05:31:10Z

Following the project's guidance that memory leaks are acceptable on unit test failure paths (as the process terminates immediately)

Just to let you know, it is no longer acceptable and will cause a failure in the CI.

Memory leaks in the failure path are acceptable, just not in the pass-path.

oops. misread. Sorry.

The unit tests for the 'detect-base64-data.c' keyword were refactored to align with coding style guidelines. Changes include: - replaced SigInit with DetectEngineAppendSig in unit tests. - variables are now declared at the point of their first use. - remove calls to SigGroupCleanup and SigCleanSignatures. Ticket: OISF#6320

victorjulien · 2025-10-17T15:37:24Z

src/detect-base64-data.c


    de_ctx->flags |= DE_QUIET;
-    de_ctx->sig_list = SigInit(de_ctx,
+    de_ctx->sig_list = DetectEngineAppendSig(de_ctx,


DetectEngineAppendSig does the list append itself, so you'd use Signature *s = DetectEngineAppendSig... here, followed by FAIL_IF_NULL(s);

jufajardini

As per the other reviewers' comments.

The function DetectEngineAppendSig() returns a pointer to the newly created Signature object. The unit tests DetectBase64DataSetupTest01 and DetectBase64DataSetupTest04 were incorrectly relying on de_ctx->sig_list to reference the parsed signature. This commit updates both tests to explicitly capture the Signature *s returned by DetectEngineAppendSig and use this specific pointer for all validation checks, ensuring test stability and correct API usage. Ticket: OISF#6320

victorjulien · 2025-10-18T06:00:45Z

src/detect-base64-data.c

-        printf("sm->type should be DETECT_BASE64_DECODE: ");
-        goto end;
-    }
+    SigMatch *sm = de_ctx->sig_list->init_data->smlists[DETECT_SM_LIST_PMATCH];


now that we have the s pointer, it's cleaner and clearer to use this here SigMatch *sm = s->init_data->smlists[DETECT_SM_LIST_PMATCH];

github-actions · 2025-10-18T06:01:44Z

NOTE: This PR may contain new authors.

…) for accessing the rule's initial data (`s->init_data`). Ticket: OISF#6320

unittests: convert detect-base64-data.c to FAIL/PASS API

4cafc0c

jhonny-rs requested a review from victorjulien as a code owner October 17, 2025 00:20

jufajardini added the outreachy Contributions made by Outreachy applicants label Oct 17, 2025

victorjulien reviewed Oct 17, 2025

View reviewed changes

inashivb reviewed Oct 17, 2025

View reviewed changes

jhonny-rs requested review from inashivb and victorjulien October 17, 2025 14:52

victorjulien reviewed Oct 17, 2025

View reviewed changes

jufajardini requested changes Oct 17, 2025

View reviewed changes

jhonny-rs force-pushed the main branch from d660062 to 87d8d7d Compare October 17, 2025 23:47

jhonny-rs requested a review from victorjulien October 18, 2025 01:54

victorjulien reviewed Oct 18, 2025

View reviewed changes

detect: Refactors DetectBase64DataSetupTest01 to use s (Signature…

3c94e4b

…) for accessing the rule's initial data (`s->init_data`). Ticket: OISF#6320

jhonny-rs requested a review from victorjulien October 18, 2025 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

unittests: convert detect-base64-data.c to FAIL/PASS API #14058

unittests: convert detect-base64-data.c to FAIL/PASS API #14058

jhonny-rs commented Oct 17, 2025

Uh oh!

github-actions bot commented Oct 17, 2025

Uh oh!

codecov bot commented Oct 17, 2025 •

edited

Loading

Uh oh!

inashivb commented Oct 17, 2025 •

edited

Loading

Uh oh!

victorjulien commented Oct 17, 2025

Uh oh!

victorjulien Oct 17, 2025

Uh oh!

jhonny-rs Oct 17, 2025

Uh oh!

inashivb left a comment

Uh oh!

inashivb commented Oct 17, 2025

Uh oh!

victorjulien Oct 17, 2025

Uh oh!

jufajardini left a comment

Uh oh!

victorjulien Oct 18, 2025

Uh oh!

github-actions bot commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

unittests: convert detect-base64-data.c to FAIL/PASS API #14058

Are you sure you want to change the base?

unittests: convert detect-base64-data.c to FAIL/PASS API #14058

Conversation

jhonny-rs commented Oct 17, 2025

Description

Testing

Uh oh!

github-actions bot commented Oct 17, 2025

Uh oh!

codecov bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

inashivb commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

victorjulien commented Oct 17, 2025

Uh oh!

victorjulien Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

jhonny-rs Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

inashivb left a comment

Choose a reason for hiding this comment

Uh oh!

inashivb commented Oct 17, 2025

Uh oh!

victorjulien Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

jufajardini left a comment

Choose a reason for hiding this comment

Uh oh!

victorjulien Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 17, 2025 •

edited

Loading

inashivb commented Oct 17, 2025 •

edited

Loading