fix null pointer dereferences found by malloc-error-check cocci - #15552
Closed
kenifor wants to merge 8 commits into
Closed
fix null pointer dereferences found by malloc-error-check cocci#15552kenifor wants to merge 8 commits into
kenifor wants to merge 8 commits into
Conversation
added 8 commits
June 4, 2026 12:30
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.
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. - Add a NULL check after SCCalloc in SCHSConfigInit; the return value was previously unguarded.
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.
catenacyber
reviewed
Jun 4, 2026
| { | ||
| MpmConfig *c = SCCalloc(1, sizeof(MpmConfig)); | ||
| if (unlikely(c == NULL)) { | ||
| FatalError("Failed to allocate MpmConfig"); |
Contributor
There was a problem hiding this comment.
You can return c, or return NULL, and caller handles it already
Author
There was a problem hiding this comment.
You can return c, or return NULL, and caller handles it already
Addressed in #15560: SCHSConfigInit now returns SCCalloc() directly without an intermediate variable, so the caller in detect-engine.c handles the NULL check as it already did.
Contributor
|
Replaced by #15560 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15552 +/- ##
==========================================
- Coverage 82.86% 82.84% -0.03%
==========================================
Files 999 999
Lines 272627 272644 +17
==========================================
- Hits 225904 225863 -41
- Misses 46723 46781 +58
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixed a broken regex in
qa/coccinelle/malloc-error-check.coccithat caused the entire script to be a no-op, then addressed all unchecked allocations it surfaced.cocci fix:
identifier func =~ "(SCMalloc|...)"uses PCRE-style syntax, but Coccinelle uses OCaml Str where|and()are literal characters —@mallocednever matched anything. Fixed by usingSCMalloc\|SCStrdup\|...syntax.allocation fixes (one commit each):
util/log-redis: guardSCCallocresult forredis_setup.stream_formatdetect/alert: guardSCStrdupresult before usedetect/flowbits: checkSCReallocresult before overwriting the original pointerdetect/reference: guard bothSCStrdupcalls inDetectReferenceParseutil/mpm-hs: remove extra parentheses causing a false cocci negative; add guard inSCHSConfigInitdecode: makePacketInitreturnbooland propagatePacketAlertCreatefailure throughPacketGetFromAllocinstead of usingFatalErroron the packet pathtests/fuzz: guardSCCallocresult infuzz_decodebase64to avoid NULL dereference on OOMFull
src/*.cscan is clean after these changes.This supersedes #15529 (rebased on current main, commits split per reviewer request).