redis: guard stream format allocation by kenifor · Pull Request #15423 · OISF/suricata

kenifor · 2026-05-20T09:08:22Z

Ticket: 8588

Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.

Contribution style:

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

I have updated the User Guide (in doc/userguide/) to reflect the changes made
I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
(including schema descriptions)
I have created a ticket at
https://redmine.openinfosecfoundation.org/projects/suricata/issues

Link to ticket: https://redmine.openinfosecfoundation.org/issues/8588

Describe changes:

Check the Redis stream format allocation before passing it to snprintf().
Use FatalError() for this unrecoverable Redis output initialization failure, matching the existing Redis setup error handling style.
Prevent a NULL pointer dereference when Redis stream/xadd mode is configured with a positive stream-maxlen and memory allocation fails.

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

To use a 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=

When Redis output is configured in stream/xadd mode with a positive stream-maxlen, SCConfLogOpenRedis() allocates redis_setup.stream_format and immediately passes it to snprintf(). If SCCalloc() fails, snprintf() receives a NULL destination pointer and the process can crash during Redis output initialization. Handle this unrecoverable setup failure with FatalError(), matching the surrounding Redis initialization error handling. Ticket: 8588

codecov · 2026-05-20T16:42:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.76%. Comparing base (bb4e79c) to head (3ca8259).
⚠️ Report is 189 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15423      +/-   ##
==========================================
+ Coverage   82.65%   82.76%   +0.11%     
==========================================
  Files         996      999       +3     
  Lines      271109   272666    +1557     
==========================================
+ Hits       224076   225674    +1598     
+ Misses      47033    46992      -41

Flag	Coverage Δ
fuzzcorpus	`61.43% <ø> (+0.40%)`	⬆️
livemode	`18.31% <ø> (-0.06%)`	⬇️
netns	`22.70% <ø> (+0.09%)`	⬆️
pcap	`44.93% <ø> (-0.29%)`	⬇️
suricata-verify	`66.40% <ø> (-0.01%)`	⬇️
unittests	`58.41% <ø> (-0.15%)`	⬇️

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.

catenacyber · 2026-05-21T08:52:27Z

            format string, whose length is limited by the length of the
            maxlen integer formatted as a string */
            log_ctx->redis_setup.stream_format = SCCalloc(100, sizeof(char));
+            if (unlikely(log_ctx->redis_setup.stream_format == NULL)) {


Why was this not caught by cocci ? ./qa/coccinelle/malloc-error-check.cocci

Good catch. The script missed it because of how ... when != x works
in Coccinelle: it excludes statements that assign to x, but not
statements that use x as an argument — so snprintf(x, ...) and
format = x both pass through the ... without triggering the
constraint. Combined with the exists semantics and the allocation
being inside a nested if (maxlen > 0), Coccinelle does not flag the
missing NULL check.

The fix we applied (adding the if (unlikely(...)) guard before the
snprintf) matches the @istested pattern and would be caught
correctly going forward. Let us know if you'd like us to also add a
test case to the cocci script to prevent similar gaps.

Could we fix the cocci script then ?

Could we fix the cocci script then ?

Fixed in 4111d98. The root cause: ... when != x in @istested only
excludes statements that reassign x, not statements that use x
as a function argument. So snprintf(x, ...) passed through the ...
undetected.

Added when != callee(..., x, ...) to the constraint so that any use
of the allocated pointer as a function argument before a NULL check
prevents @istested from matching — causing the script to correctly
flag the violation.

Thanks, checking with #15508 that this cocci change is good

Also, we would like to have a shorter commit title, see https://docs.suricata.io/en/latest/devguide/contributing/code-submission-process.html#commits

catenacyber · 2026-05-28T09:17:36Z

Also, we need you yo sign the CLA https://suricata.io/about/contribution-agreement/ to be able to accept your contribution

…unction args The @istested rule's '... when != x' only excluded reassignments of x, not statements that passed x as a function argument. Add 'when != callee(..., x, ...)' so that any use of the allocated pointer as a function argument before a NULL check is no longer treated as tested and is correctly flagged.

kenifor · 2026-05-28T14:13:36Z

Also, we need you yo sign the CLA https://suricata.io/about/contribution-agreement/ to be able to accept your contribution

yes, I signed it now

github-actions · 2026-05-28T14:19:34Z

NOTE: This PR may contain new authors.

catenacyber

The cocci commit does not work : see https://github.com/OISF/suricata/actions/runs/26811739736/job/79043416837?pr=15508 from #15508

The identifier regex used PCRE-style '(A|B)' syntax, but Coccinelle uses OCaml Str where '|' is a literal character. Replace with 'A\|B' so the pattern correctly matches all SC*alloc functions.

kenifor · 2026-06-02T13:27:26Z

The cocci commit does not work : see https://github.com/OISF/suricata/actions/runs/26811739736/job/79043416837?pr=15508 from #15508

Fixed in 3ca8259. The root cause was deeper: the entire script was a no-op because identifier =~ "(SCMalloc|SCCalloc|...)" uses PCRE-style syntax, but Coccinelle uses OCaml Str where | and () are literal characters — so @malloced never matched anything.

Fixed by replacing all (A|B) patterns with A\|B (OCaml Str alternation). After this fix the script correctly catches the missing null check: exit 1 on the buggy code, exit 0 on the fixed version.

catenacyber · 2026-06-02T14:45:47Z

Cool, but now we see more reports like

Structure malloced at ../../src/detect-reference.c:155 but error is not checked.

Could you fix them also ?

github-actions · 2026-06-02T14:58:12Z

NOTE: This PR may contain new authors.

- decode: replace DEBUG_VALIDATE_BUG_ON with FatalError in PacketAlertCreate - detect-engine-alert: guard SCStrdup result before use - detect-flowbits: check SCRealloc result before overwriting original pointer - detect-reference: guard SCStrdup results in DetectReferenceParse - util-mpm-hs: fix false cocci negative (remove extra parens), add FatalError in SCHSConfigInit

kenifor · 2026-06-03T08:30:02Z

Cool, but now we see more reports like
Structure malloced at ../../src/detect-reference.c:155 but error is not checked.
Could you fix them also ?

Fixed in 207594e. The repaired cocci script surfaced 6 unchecked allocations across 5 files:

decode.c: replaced DEBUG_VALIDATE_BUG_ON with FatalError in PacketAlertCreate
detect-engine-alert.c: guard SCStrdup result before use
detect-flowbits.c: check SCRealloc result before overwriting the original pointer
detect-reference.c: guard both SCStrdup calls in DetectReferenceParse
util-mpm-hs.c: remove extra parentheses in existing check (false negative), add FatalError in SCHSConfigInit

Full scan of src/*.c is now clean.

github-actions · 2026-06-03T09:11:38Z

Closing this pull request: its branch was updated after the pull
request was opened.

Per our workflow, a new pull request is required when changes are made
to an existing one. Please open a new pull request with the updated
changes.

If you wish to create an in progress pull request that you can push to,
please create a draft pull request.

Please see our GitHub Pull Request Workflow.

kenifor · 2026-06-03T10:37:50Z

Cool, but now we see more reports like
Structure malloced at ../../src/detect-reference.c:155 but error is not checked.
Could you fix them also ?

Continued in #15529 (PR was auto-closed after branch update per the workflow policy).

kenifor requested a review from victorjulien as a code owner May 20, 2026 09:08

victorjulien added the cla required The author has not yet signed the CLA or CLA signing is pending verification label May 20, 2026

catenacyber reviewed May 21, 2026

View reviewed changes

kenifor requested a review from a team as a code owner May 28, 2026 14:11

catenacyber mentioned this pull request Jun 2, 2026

DRAFT: check cocci improvement catches missing NULL check #15508

Closed

catenacyber requested changes Jun 2, 2026

View reviewed changes

qa/cocci: fix broken regex alternation in malloc-error-check

3ca8259

The identifier regex used PCRE-style '(A|B)' syntax, but Coccinelle uses OCaml Str where '|' is a literal character. Replace with 'A\|B' so the pattern correctly matches all SC*alloc functions.

github-actions Bot closed this Jun 3, 2026

kenifor mentioned this pull request Jun 3, 2026

fix null pointer dereferences found by malloc-error-check cocci #15529

Closed

Uh oh!

Conversation

kenifor commented May 20, 2026

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Uh oh!

codecov Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

catenacyber May 21, 2026

Choose a reason for hiding this comment

Uh oh!

kenifor May 26, 2026

Choose a reason for hiding this comment

Uh oh!

catenacyber May 28, 2026

Choose a reason for hiding this comment

Uh oh!

kenifor May 28, 2026

Choose a reason for hiding this comment

Uh oh!

catenacyber Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

catenacyber Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

catenacyber commented May 28, 2026

Uh oh!

kenifor commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 28, 2026

Uh oh!

catenacyber left a comment

Choose a reason for hiding this comment

Uh oh!

kenifor commented Jun 2, 2026

Uh oh!

catenacyber commented Jun 2, 2026

Uh oh!

github-actions Bot commented Jun 2, 2026

Uh oh!

kenifor commented Jun 3, 2026

Uh oh!

github-actions Bot commented Jun 3, 2026

Uh oh!

kenifor commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 20, 2026 •

edited

Loading

kenifor commented May 28, 2026 •

edited

Loading