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.
pull/16329/head
Victor Julien 4 weeks ago
parent 9a37bb659b
commit 32ccee91c3

@ -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)

@ -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);

@ -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) {

@ -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

@ -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);

Loading…
Cancel
Save