Skip to content

redis: guard stream format allocation #15423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kenifor wants to merge 4 commits into from
Closed

redis: guard stream format allocation #15423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8abe674
redis: guard stream format allocation
May 14, 2026
4111d98
qa: fix cocci malloc-error-check to catch use-before-null-check via f…
May 28, 2026
3ca8259
qa/cocci: fix broken regex alternation in malloc-error-check
Jun 2, 2026
207594e
fix null pointer dereferences found by malloc-error-check cocci
Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions qa/coccinelle/malloc-error-check.cocci
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
@malloced@
expression x;
position p1;
identifier func =~ "(SCMalloc|SCStrdup|SCCalloc|SCMallocAligned|SCRealloc)";
identifier func =~ "SCMalloc\|SCStrdup\|SCCalloc\|SCMallocAligned\|SCRealloc";
@@

x@p1 = func(...)
Expand All @@ -10,7 +10,7 @@ x@p1 = func(...)
expression x, E;
statement S;
position malloced.p1;
identifier func =~ "(SCMalloc|SCStrdup|SCCalloc|SCMallocAligned|SCRealloc)";
identifier func =~ "SCMalloc\|SCStrdup\|SCCalloc\|SCMallocAligned\|SCRealloc";
@@

(
Expand All @@ -22,7 +22,7 @@ if (E && (x@p1 = func(...)) == NULL) S
@realloc exists@
position malloced.p1;
expression x, E1;
identifier func =~ "(SCMalloc|SCCalloc|SCMallocAligned)";
identifier func =~ "SCMalloc\|SCCalloc\|SCMallocAligned";
@@

x@p1 = func(...)
Expand All @@ -33,7 +33,7 @@ x = SCRealloc(x, E1)
expression x, E1;
position malloced.p1;
statement S1, S2;
identifier func =~ "(SCMalloc|SCStrdup|SCCalloc|SCMallocAligned|SCRealloc)";
identifier func =~ "SCMalloc\|SCStrdup\|SCCalloc\|SCMallocAligned\|SCRealloc";
@@

x@p1 = func(...)
Expand Down
4 changes: 3 additions & 1 deletion src/decode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ ExceptionPolicyStatsSetts flow_memcap_eps_stats = {
PacketAlert *PacketAlertCreate(void)
{
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");
}

return pa_array;
}
Expand Down
3 changes: 3 additions & 0 deletions src/detect-engine-alert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,9 @@ static inline int PacketAlertSetContext(
}
}
current_json->json_string = SCStrdup(det_ctx->json_content[i].json_content);
if (current_json->json_string == NULL) {
return -1;
}
SCLogDebug("json content %u, value '%s' (%p)", (unsigned int)i,
current_json->json_string, s);
}
Expand Down
4 changes: 2 additions & 2 deletions src/detect-flowbits.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,11 +791,11 @@ int DetectFlowbitsAnalyze(DetectEngineCtx *de_ctx)
uint32_t new_fb_array_size = s->init_data->rule_state_flowbits_ids_size + 1;
void *tmp_fb_ptr = SCRealloc(s->init_data->rule_state_flowbits_ids_array,
new_fb_array_size * sizeof(uint32_t));
s->init_data->rule_state_flowbits_ids_array = tmp_fb_ptr;
if (s->init_data->rule_state_flowbits_ids_array == NULL) {
if (tmp_fb_ptr == NULL) {
SCLogError("Failed to reallocate memory for rule_state_variable_idx");
goto error;
}
s->init_data->rule_state_flowbits_ids_array = tmp_fb_ptr;
SCLogDebug(
"realloc'ed array for flowbits ids, new size is %u", new_fb_array_size);
s->init_data->rule_state_dependant_sids_size = new_array_size;
Expand Down
6 changes: 6 additions & 0 deletions src/detect-reference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,19 @@ static DetectReference *DetectReferenceParse(const char *rawstr, DetectEngineCtx
if (strlen(scheme)) {
SCLogConfig("scheme value %s overrides key %s", scheme, key);
ref->key = SCStrdup(scheme);
if (ref->key == NULL) {
goto error;
}
/* already bound checked to be REFERENCE_SYSTEM_NAME_MAX or less */
ref->key_len = (uint16_t)strlen(scheme);
} else {

SCRConfReference *lookup_ref_conf = SCRConfGetReference(key, de_ctx);
if (lookup_ref_conf != NULL) {
ref->key = SCStrdup(lookup_ref_conf->url);
if (ref->key == NULL) {
goto error;
}
/* already bound checked to be REFERENCE_SYSTEM_NAME_MAX or less */
ref->key_len = (uint16_t)strlen(ref->key);
} else {
Expand Down
3 changes: 3 additions & 0 deletions src/util-log-redis.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,9 @@ int SCConfLogOpenRedis(SCConfNode *redis_node, void *lf_ctx)
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)) {

@catenacyber catenacyber May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kenifor kenifor May 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@catenacyber catenacyber May 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we fix the cocci script then ?

@kenifor kenifor May 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@catenacyber catenacyber Jun 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@catenacyber catenacyber Jun 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FatalError("Unable to allocate redis stream format");
}
snprintf(log_ctx->redis_setup.stream_format, 100, redis_stream_format_maxlen_tmpl, "%s",
"%s", exact ? '=' : '~', maxlen, "%s");
log_ctx->redis_setup.format = log_ctx->redis_setup.stream_format;
Expand Down
5 changes: 4 additions & 1 deletion src/util-mpm-hs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ static int CompileDataExtensionsInit(hs_expr_ext_t **ext, const SCHSPattern *p)
{
if (p->flags & (MPM_PATTERN_FLAG_OFFSET | MPM_PATTERN_FLAG_DEPTH)) {
*ext = SCCalloc(1, sizeof(hs_expr_ext_t));
if ((*ext) == NULL) {
if (*ext == NULL) {
return -1;
}
if (p->flags & MPM_PATTERN_FLAG_OFFSET) {
Expand Down Expand Up @@ -1179,6 +1179,9 @@ void SCHSPrintInfo(MpmCtx *mpm_ctx)
static MpmConfig *SCHSConfigInit(void)
{
MpmConfig *c = SCCalloc(1, sizeof(MpmConfig));
if (unlikely(c == NULL)) {
FatalError("Failed to allocate MpmConfig");
}
return c;
}

Expand Down
Loading