fix null pointer dereferences found by malloc-error-check cocci - #15529
Closed
kenifor wants to merge 5 commits into
Closed
fix null pointer dereferences found by malloc-error-check cocci#15529kenifor wants to merge 5 commits into
kenifor wants to merge 5 commits into
Conversation
added 4 commits
May 20, 2026 12:07
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
…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.
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.
- 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
5 tasks
Member
|
These are nice finds. Can you split the last commit "fix null pointer dereferences found by malloc-error-check cocci" to have a commit per logical unit, like |
|
NOTE: This PR may contain new authors. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15529 +/- ##
==========================================
+ Coverage 82.65% 82.84% +0.18%
==========================================
Files 996 999 +3
Lines 271109 272618 +1509
==========================================
+ Hits 224076 225841 +1765
+ Misses 47033 46777 -256
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Member
|
@catenacyber remaining warning: Do you think this needs to be addressed? Or should we exclude the fuzz targets? |
catenacyber
reviewed
Jun 3, 2026
| PacketAlert *pa_array = SCCalloc(packet_alert_max, sizeof(PacketAlert)); | ||
| DEBUG_VALIDATE_BUG_ON(pa_array == NULL); | ||
| if (unlikely(pa_array == NULL)) { | ||
| FatalError("Failed to allocate packet alert array"); |
Contributor
There was a problem hiding this comment.
Cannot have FatalError in the packet path
Is the good fix to have this allocated at the same time than the Packet structure ?
PacketAlertCreate is called from PacketInit which is on the packet allocation path. Using FatalError there is inappropriate — on allocation failure the packet should be dropped, not the process terminated. 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.
Contributor
|
Replaced by #15552 |
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
3 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
qa/coccinelle/malloc-error-check.cocci:identifier func =~ "(SCMalloc|...)"uses PCRE-style syntax, but Coccinelle uses OCaml Str where|and()are literal — so@mallocednever matched anything. Replaced withSCMalloc\|SCStrdup\|...syntax.src/util-log-redis.c: guardSCCallocresult forstream_formatsrc/decode.c: replaceDEBUG_VALIDATE_BUG_ONwithFatalErrorinPacketAlertCreate(debug-only check was a no-op in production)src/detect-engine-alert.c: guardSCStrdupresult before usesrc/detect-flowbits.c: checkSCReallocresult before overwriting the original pointersrc/detect-reference.c: guard bothSCStrdupcalls withgoto errorsrc/util-mpm-hs.c: remove extra parentheses causing false negative; addFatalErrorinSCHSConfigInitFull
src/*.cscan is clean after these changes.This is a continuation of #15423 (closed per the GitHub PR workflow policy after branch was updated).