fix null pointer dereferences found by malloc-error-check cocci by kenifor · Pull Request #15560 · OISF/suricata

kenifor · 2026-06-04T13:36:04Z

Supersedes #15552. Updated util/mpm-hs commit: instead of FatalError,
SCHSConfigInit now returns SCCalloc() directly — the caller in
detect-engine.c already checks the return value for NULL.

@malloced

The identifier regex used PCRE-style '(A|B)' syntax, but Coccinelle uses OCaml Str where '|' and '()' are literal characters. The @malloced rule never matched any SC*alloc call, making the entire script a no-op. Replace all five identifier =~ patterns with the correct OCaml Str alternation syntax 'A\|B'.

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

SCStrdup result was stored and immediately used without checking for NULL, which would cause a NULL dereference if allocation fails.

The original pointer was overwritten with the SCRealloc result before checking for NULL, causing a memory leak if reallocation fails. Check the temporary pointer first before assigning.

Two SCStrdup calls that set ref->key had no NULL check. On allocation failure the pointer would be used immediately, causing a NULL dereference.

PacketAlertCreate is called from PacketInit on the packet allocation path. Crashing the process on allocation failure is inappropriate here; the caller should be able to drop the packet gracefully. Make PacketInit return bool and propagate the NULL result from PacketAlertCreate up through PacketGetFromAlloc, which already returns NULL to signal allocation failure to its callers. Update the four UNITTESTS-only helpers in defrag.c accordingly.

If SCCalloc fails, decoded is NULL and the subsequent SCBase64Decode call would dereference it. Return early on allocation failure.

Two fixes: - Remove extra parentheses in existing NULL check: (*ext) -> *ext, which was causing the cocci script to miss the check as a false negative. - Simplify SCHSConfigInit to return SCCalloc() directly; the caller in detect-engine.c already checks the return value for NULL.

catenacyber

Thanks for the work, I think it needs a dedicated ticket for the qa/cocci fix

CI : ✅
Git ID set : looks fine for me
CLA : did you sign it ?
Doc update : not needed
Redmine ticket : I think we should have one for the cocci/QA fix, and rather a distinct one from https://redmine.openinfosecfoundation.org/issues/8588 which can include the other fixes...
Rustfmt : no rust
Tests : this is nice
Dependencies added: none
Code : good
Commits segmentation : ok cool
Commit messages : ok, I find them too long/verbose

Do you know if this was always broken ? or broken by a coccinelle update ?

catenacyber · 2026-06-04T14:17:47Z

            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)) {
+                FatalError("Unable to allocate redis stream format");


@jasonish is this ok to FatalError here ?

Yes, I think it as a relevant to a fatal error as the ones just below it.

catenacyber · 2026-06-04T14:18:48Z

-    DEBUG_VALIDATE_BUG_ON(pa_array == NULL);
-
-    return pa_array;
+    return SCCalloc(packet_alert_max, sizeof(PacketAlert));


The commit message should not mention FatalError which is not the case in main right now

jufajardini · 2026-06-04T14:36:53Z

+    if (!PacketInit(p)) {
+        SCFree(p);
+        return NULL;
+    }


I don't know what we prefer, if this approach or the one in the next function (line 1228), but we should be consistent.

jufajardini

Thanks for your work! I've pointed some inconsistencies I think should be addressed, since we'll have another iteration.

jufajardini · 2026-06-04T14:38:19Z

    if (IPV6_GET_RAW_VER(ip6p) != 6)
        goto error;
    if (IPV6_GET_RAW_NH(ip6p) != 44)
        goto error;
    if (IPV6_GET_RAW_PLEN(ip6p) != sizeof(IPV6FragHdr) + content_len)
        goto error;



Same here, I think: some tests are using FAIL_IF, some are not...

github-actions · 2026-06-04T16:30:36Z

NOTE: This PR may contain new authors.

codecov · 2026-06-04T18:15:37Z

Codecov Report

❌ Patch coverage is 55.55556% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.85%. Comparing base (0eda09f) to head (2f60a59).
⚠️ Report is 91 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15560      +/-   ##
==========================================
- Coverage   82.86%   82.85%   -0.01%     
==========================================
  Files         999      999              
  Lines      272627   272641      +14     
==========================================
- Hits       225904   225891      -13     
- Misses      46723    46750      +27

Flag	Coverage Δ
fuzzcorpus	`61.47% <40.00%> (-0.01%)`	⬇️
livemode	`18.43% <26.66%> (+0.02%)`	⬆️
netns	`22.80% <26.66%> (-0.03%)`	⬇️
pcap	`45.16% <33.33%> (-0.05%)`	⬇️
suricata-verify	`66.55% <60.00%> (-0.03%)`	⬇️
unittests	`58.48% <40.74%> (-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.

kenifor · 2026-06-09T08:25:03Z

Thanks for the work, I think it needs a dedicated ticket for the qa/cocci fix

CI : ✅ Git ID set : looks fine for me CLA : did you sign it ? Doc update : not needed Redmine ticket : I think we should have one for the cocci/QA fix, and rather a distinct one from https://redmine.openinfosecfoundation.org/issues/8588 which can include the other fixes... Rustfmt : no rust Tests : this is nice Dependencies added: none Code : good Commits segmentation : ok cool Commit messages : ok, I find them too long/verbose

Do you know if this was always broken ? or broken by a coccinelle update ?

Addressed in #15584: added Ticket: 8641 for the cocci fix, corrected the decode commit message, and clarified the defrag.c helpers (different styles reflect different return types: Packet * vs int).

Re: "was this always broken?" — yes, from the beginning. The (A|B) syntax is valid PCRE but not OCaml Str (Coccinelle's regex engine), so the script never matched anything regardless of Coccinelle version.

catenacyber · 2026-06-23T12:22:17Z

Merged by #15584

Denis Balashov added 8 commits June 4, 2026 16:34

detect/alert: guard SCStrdup result before use

62cbdcf

SCStrdup result was stored and immediately used without checking for NULL, which would cause a NULL dereference if allocation fails.

detect/flowbits: check SCRealloc result before overwriting pointer

ae4fa81

The original pointer was overwritten with the SCRealloc result before checking for NULL, causing a memory leak if reallocation fails. Check the temporary pointer first before assigning.

detect/reference: guard SCStrdup calls in DetectReferenceParse

2690c5c

Two SCStrdup calls that set ref->key had no NULL check. On allocation failure the pointer would be used immediately, causing a NULL dereference.

tests/fuzz: guard SCCalloc result in fuzz_decodebase64

d54ce83

If SCCalloc fails, decoded is NULL and the subsequent SCBase64Decode call would dereference it. Return early on allocation failure.

kenifor requested review from a team and victorjulien as code owners June 4, 2026 13:36

kenifor mentioned this pull request Jun 4, 2026

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

Closed

catenacyber mentioned this pull request Jun 4, 2026

detect/parse: guard HashListTableLookup results in duplicate sig check #15557

Closed

5 tasks

catenacyber requested changes Jun 4, 2026

View reviewed changes

catenacyber reviewed Jun 4, 2026

View reviewed changes

jufajardini reviewed Jun 4, 2026

View reviewed changes

kenifor mentioned this pull request Jun 9, 2026

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

Closed

catenacyber closed this Jun 23, 2026

Navigation Menu

Uh oh!

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

fix null pointer dereferences found by malloc-error-check cocci#15560
kenifor wants to merge 8 commits into
OISF:mainfrom
kenifor:fix-null-deref-malloc-checks-v2

kenifor commented Jun 4, 2026

Uh oh!

catenacyber left a comment

Uh oh!

catenacyber Jun 4, 2026

Uh oh!

jasonish Jun 5, 2026

Uh oh!

catenacyber Jun 4, 2026

Uh oh!

jufajardini Jun 4, 2026

Uh oh!

jufajardini left a comment

Uh oh!

jufajardini Jun 4, 2026

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

codecov Bot commented Jun 4, 2026 •

edited

Loading

Uh oh!

kenifor commented Jun 9, 2026

Uh oh!

catenacyber commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

kenifor commented Jun 4, 2026

Uh oh!

catenacyber left a comment

Choose a reason for hiding this comment

Uh oh!

catenacyber Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

jasonish Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

catenacyber Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

jufajardini Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

jufajardini left a comment

Choose a reason for hiding this comment

Uh oh!

jufajardini Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

kenifor commented Jun 9, 2026

Uh oh!

catenacyber commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jun 4, 2026 •

edited

Loading