From 32ccee91c3a1e583a7c83e74f8dd7060067e796d Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Tue, 29 Sep 2026 22:02:42 +0000 Subject: [PATCH] detect/alert: release the alert context not carried by the packet PacketAlertSetContext() allocates the "alert:" pcre capture context for every alert a detection thread queues, but only alerts copied into the packet were ever freed. Entries the queue dropped instead kept owning it: threshold and suppression hits, alerts over packet_alert_max, skip_td and skip_fw bypasses, and everything after a pass rule broke out of the loop. Under ASAN, 500 suppressed packets leaked 19461 bytes in 998 allocations. The copy AlertQueueAppend() keeps in `p->alerts.drop` owns a context of its own and is not an alerts[] slot, so the packet side recycler never reached it. Two sites invalidate that slot: PacketReinit() and the per packet reset in DetectRunSetup(). Both only cleared `action`, and the second is the worse one: a slot that reads as empty while it still owns memory lets the next drop alert overwrite the pointer and lose the chain. The assignment in AlertQueueAppend() needs no release of its own, as its `p->alerts.drop.action == 0` guard only passes for a slot one of the two reset sites already emptied. Clear the queue entry where the alert is handed to the packet, and release whatever the remaining entries own at the end of PacketAlertFinalizeProcessQueue() so every exit path runs through it. Route both drop-slot reset sites through PacketAlertDropReset(), which releases the chain first: the slot's own pointer is the guard, not PKT_ALERT_CTX_USED, a whole packet hint set at PacketAlertFinalize() and stale at the reset sites. Packets that are freed rather than recycled (PacketFree() -> PacketDestructor(), e.g. at pool teardown) release their drop copy only via the PacketDestructor site. Ticket: #9116. --- src/decode.c | 10 ++++++++++ src/decode.h | 2 ++ src/detect-engine-alert.c | 37 +++++++++++++++++++++++++++++++++++++ src/detect.c | 2 +- src/packet.c | 3 ++- 5 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/decode.c b/src/decode.c index 9d7b2df244..d4ae6bed70 100644 --- a/src/decode.c +++ b/src/decode.c @@ -163,6 +163,16 @@ void PacketAlertRecycle(PacketAlert *pa_array, uint16_t cnt) } } +/** \brief Release the alert context the drop slot owns, then invalidate the slot. + * Not an `alerts[]` entry, so clearing `action` alone leaves it owning the chain; + * `action` is the guard consumers read, so a leaked slot still reads as empty. + */ +void PacketAlertDropReset(Packet *p) +{ + PacketAlertRecycle(&p->alerts.drop, 1); + p->alerts.drop.action = 0; +} + void PacketAlertFree(PacketAlert *pa_array) { if (pa_array == NULL) diff --git a/src/decode.h b/src/decode.h index fb7b1466ef..0d78285751 100644 --- a/src/decode.h +++ b/src/decode.h @@ -296,8 +296,10 @@ typedef struct PacketAlerts_ { PacketAlert drop; } PacketAlerts; +struct Packet_; PacketAlert *PacketAlertCreate(void); void PacketAlertRecycle(PacketAlert *pa_array, uint16_t cnt); +void PacketAlertDropReset(struct Packet_ *p); void PacketAlertFree(PacketAlert *pa); diff --git a/src/detect-engine-alert.c b/src/detect-engine-alert.c index 0316cee17c..3074b10520 100644 --- a/src/detect-engine-alert.c +++ b/src/detect-engine-alert.c @@ -370,6 +370,34 @@ static inline int PacketAlertSetContext( return 0; } +/** \internal + * \brief Free the alert context a PacketAlert owns and clear the reference. + */ +static void PacketAlertContextRelease(PacketAlert *pa) +{ + struct PacketContextData *json = pa->json_info; + while (json != NULL) { + struct PacketContextData *next = json->next; + SCFree(json->json_string); + SCFree(json); + json = next; + } + pa->json_info = NULL; +} + +/** \internal + * \brief Release the alert context of queue entries for alerts that never reached the packet. + */ +static void AlertQueueReleaseContext(DetectEngineThreadCtx *det_ctx) +{ + for (uint16_t i = 0; i < det_ctx->alert_queue_size; i++) { + PacketAlert *pa = &det_ctx->alert_queue[i]; + if (pa->json_info != NULL) { + PacketAlertContextRelease(pa); + } + } +} + /** \internal */ static inline PacketAlert PacketAlertSet(DetectEngineThreadCtx *det_ctx, const Signature *s, @@ -397,6 +425,9 @@ static void AlertQueueAppend(DetectEngineThreadCtx *det_ctx, const Signature *s, /* first time we see a drop action signature, set that in the packet */ /* we do that even before inserting into the queue, so we save it even if appending fails */ if (p->alerts.drop.action == 0 && s->action & ACTION_DROP) { + /* an empty drop slot owns no context: both reset sites release the chain + * before clearing `action`, so overwriting the whole slot here is safe. */ + DEBUG_VALIDATE_BUG_ON(p->alerts.drop.json_info != NULL); p->alerts.drop = PacketAlertSet(det_ctx, s, tx_id, sub_state, alert_flags); SCLogDebug("sid %u: set PacketAlert drop action. s->iid %" PRIu32 "", s->id, s->iid); } @@ -590,6 +621,7 @@ static struct DetectFirewallPolicy HandleFirewallRule( if (s->action & ACTION_ALERT) { if (p->alerts.cnt < packet_alert_max) { p->alerts.alerts[p->alerts.cnt++] = *pa; + pa->json_info = NULL; // ownership moved to the packet } else { p->alerts.firewall_discarded++; } @@ -761,6 +793,7 @@ static inline void PacketAlertFinalizeProcessQueue( p->alerts.suppressed++; } else if (p->alerts.cnt < packet_alert_max) { p->alerts.alerts[p->alerts.cnt++] = *pa; + pa->json_info = NULL; // ownership moved to the packet SCLogDebug("appending sid %" PRIu32 " alert to Packet::alerts at pos %u; action:%02x", s->id, i, pa->action); @@ -786,6 +819,10 @@ static inline void PacketAlertFinalizeProcessQueue( } fw_dropped: + /* release the context of the alerts we did not hand over to the packet: + * suppressed, discarded, skipped or simply never reached. */ + AlertQueueReleaseContext(det_ctx); + /* after threat detection has been handled, see if the fw intended to accept (drop is handled * immediately by the fw), as fw accept can be overruled by td drop. */ if (have_fw_rules) { diff --git a/src/detect.c b/src/detect.c index 4e5e41d5f4..414a7680b3 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1041,7 +1041,7 @@ static DetectRunScratchpad DetectRunSetup(const DetectEngineCtx *de_ctx, det_ctx->json_content_len = 0; det_ctx->alert_queue_size = 0; - p->alerts.drop.action = 0; + PacketAlertDropReset(p); p->alerts.firewall_discarded = 0; #ifdef DEBUG diff --git a/src/packet.c b/src/packet.c index f2847c3bc4..bb19ffa85a 100644 --- a/src/packet.c +++ b/src/packet.c @@ -142,7 +142,7 @@ void PacketReinit(Packet *p) p->alerts.discarded = 0; p->alerts.firewall_discarded = 0; p->alerts.suppressed = 0; - p->alerts.drop.action = 0; + PacketAlertDropReset(p); if (p->alerts.cnt > 0) { if (pflags & PKT_ALERT_CTX_USED) PacketAlertRecycle(p->alerts.alerts, p->alerts.cnt); @@ -178,6 +178,7 @@ void PacketDestructor(Packet *p) if (p->pktvar != NULL) { PktVarFree(p->pktvar); } + PacketAlertRecycle(&p->alerts.drop, 1); PacketAlertFree(p->alerts.alerts); PACKET_FREE_EXTDATA(p); SCSpinDestroy(&p->persistent.tunnel_lock);