Skip to content

Cleanups/v5 #15725

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
victorjulien wants to merge 12 commits into from
+54 −81
Closed

Cleanups/v5 #15725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7a07b53
htp: remove duplicate entries in the event table
victorjulien Jun 13, 2026
6528a3c
detect/file: remove unused registration fields
victorjulien Jun 8, 2026
61282d7
detect/file: minor cleanup
victorjulien Jun 8, 2026
465cdbf
detect/file.data: reduce scope for MPM datatype
victorjulien Jun 13, 2026
11e4cd3
detect/file: rename file registration type
victorjulien Jun 8, 2026
c39cc31
detect/parse: tighten hook parsing
victorjulien Jun 10, 2026
36d7c67
app-layer: minor code cleanup for GetStateProgress
victorjulien Jun 12, 2026
d306c86
app-layer/parser: remove misleading comment
victorjulien Jun 12, 2026
ddff96e
app-layer: cleanup inspect id getter
victorjulien Jun 13, 2026
eb1bba7
app-layer: enforce pstate as non-NULL
victorjulien Jun 13, 2026
3dbf732
app-layer: treat pstate as non-NULL
victorjulien Jun 13, 2026
3d231c8
http/xff: harden code against http in detection-only
victorjulien Jun 25, 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
9 changes: 3 additions & 6 deletions src/app-layer-htp-xff.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,17 +182,14 @@ int HttpXFFGetIPFromTx(
*/
int HttpXFFGetIP(const Flow *f, HttpXFFCfg *xff_cfg, char *dstbuf, int dstbuflen)
{
HtpState *htp_state = NULL;
uint64_t tx_id = AppLayerParserGetMinId(f->alparser);
uint64_t total_txs = 0;

htp_state = (HtpState *)FlowGetAppState(f);
HtpState *htp_state = (HtpState *)FlowGetAppState(f);
if (htp_state == NULL) {
SCLogDebug("no http state, XFF IP cannot be retrieved");
goto end;
}

total_txs = AppLayerParserGetTxCnt(f, htp_state);
uint64_t tx_id = AppLayerParserGetMinId(f->alparser);
const uint64_t total_txs = AppLayerParserGetTxCnt(f, htp_state);
AppLayerGetTxIteratorFunc IterFunc = AppLayerGetTxIterator(f->proto, f->alproto);
AppLayerGetTxIterState state;
memset(&state, 0, sizeof(state));
Expand Down
6 changes: 0 additions & 6 deletions src/app-layer-htp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,19 +165,13 @@ SCEnumCharMap http_decoder_event_table[] = {
"CONTENT_LENGTH_EXTRA_DATA_END",
HTP_LOG_CODE_CONTENT_LENGTH_EXTRA_DATA_END,
},
{
"CONTENT_LENGTH_EXTRA_DATA_END",
HTP_LOG_CODE_CONTENT_LENGTH_EXTRA_DATA_END,
},
{ "SWITCHING_PROTO_WITH_CONTENT_LENGTH", HTP_LOG_CODE_SWITCHING_PROTO_WITH_CONTENT_LENGTH },
{ "DEFORMED_EOL", HTP_LOG_CODE_DEFORMED_EOL },
{ "PARSER_STATE_ERROR", HTP_LOG_CODE_PARSER_STATE_ERROR },
{ "MISSING_OUTBOUND_TRANSACTION_DATA", HTP_LOG_CODE_MISSING_OUTBOUND_TRANSACTION_DATA },
{ "MISSING_INBOUND_TRANSACTION_DATA", HTP_LOG_CODE_MISSING_INBOUND_TRANSACTION_DATA },
{ "MISSING_INBOUND_TRANSACTION_DATA", HTP_LOG_CODE_MISSING_INBOUND_TRANSACTION_DATA },
{ "ZERO_LENGTH_DATA_CHUNKS", HTP_LOG_CODE_ZERO_LENGTH_DATA_CHUNKS },
{ "REQUEST_LINE_UNKNOWN_METHOD", HTP_LOG_CODE_REQUEST_LINE_UNKNOWN_METHOD },
{ "REQUEST_LINE_UNKNOWN_METHOD", HTP_LOG_CODE_REQUEST_LINE_UNKNOWN_METHOD },
{ "REQUEST_LINE_UNKNOWN_METHOD_NO_PROTOCOL",
HTP_LOG_CODE_REQUEST_LINE_UNKNOWN_METHOD_NO_PROTOCOL },
{ "REQUEST_LINE_UNKNOWN_METHOD_INVALID_PROTOCOL",
Expand Down
54 changes: 19 additions & 35 deletions src/app-layer-parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,36 +719,30 @@ AppLayerGetTxIteratorFunc AppLayerGetTxIterator(const uint8_t ipproto,
uint64_t AppLayerParserGetTransactionLogId(AppLayerParserState *pstate)
{
SCEnter();

SCReturnCT((pstate == NULL) ? 0 : pstate->log_id, "uint64_t");
DEBUG_VALIDATE_BUG_ON(pstate == NULL);
SCReturnCT(pstate->log_id, "uint64_t");
}

uint64_t AppLayerParserGetMinId(AppLayerParserState *pstate)
{
SCEnter();

SCReturnCT((pstate == NULL) ? 0 : pstate->min_id, "uint64_t");
DEBUG_VALIDATE_BUG_ON(pstate == NULL);
SCReturnCT(pstate->min_id, "uint64_t");

@jasonish jasonish Jun 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can still get here with a NULL...

[Switching to Thread 0x7bffcf9ff6c0 (LWP 1703692)]
0x000055555609e6f8 in AppLayerParserGetMinId (pstate=0x0) at app-layer-parser.c:730
730	   SCReturnCT(pstate->min_id, "uint64_t");
(gdb) bt
#0  0x000055555609e6f8 in AppLayerParserGetMinId (pstate=0x0) at app-layer-parser.c:730
#1  0x0000555556906a4f in HttpXFFGetIP (f=0x7d1ff55ad6c0, xff_cfg=0x7c1ff527b910, dstbuf=0x7bffce9f08d0 "", dstbuflen=46)
    at app-layer-htp-xff.c:186
#2  0x0000555556a8bda4 in AlertJson (tv=0x7d1ff58209c0, aft=0x7c2ff547b510, p=0x7dcff6edf280) at output-json-alert.c:724
#3  0x0000555556a8d327 in JsonAlertLogger (tv=0x7d1ff58209c0, thread_data=0x7c2ff547b510, p=0x7dcff6edf280) at output-json-alert.c:915
#4  0x0000555556ab0188 in OutputPacketLog (tv=0x7d1ff58209c0, p=0x7dcff6edf280, thread_data=0x7c1ff52aaa10) at output-packet.c:104
#5  0x00005555566adb85 in OutputLoggerLog (tv=0x7d1ff58209c0, p=0x7dcff6edf280, thread_data=0x7c1ff52aaa90) at output.c:790
#6  0x0000555556667f23 in FlowWorkerFlowTimeout (tv=0x7d1ff58209c0, p=0x7dcff6edf280, fw=0x7cfff5260040, det_ctx=0x7d6ff5370200)
    at flow-worker.c:462
#7  0x000055555666586e in FlowFinish (tv=0x7d1ff58209c0, f=0x7d1ff55ad6c0, fw=0x7cfff5260040, detect_thread=0x7d6ff5370200)
    at flow-worker.c:129
#8  0x0000555556665d8d in CheckWorkQueue (tv=0x7d1ff58209c0, fw=0x7cfff5260040, counters=0x7bffce76d320, fq=0x7cfff52600a8, max_work=0)
    at flow-worker.c:170
#9  0x00005555566686da in FlowWorkerProcessLocalFlows (tv=0x7d1ff58209c0, fw=0x7cfff5260040, p=0x7dcff8d49680) at flow-worker.c:513
#10 0x000055555666aa3c in FlowWorker (tv=0x7d1ff58209c0, p=0x7dcff8d49680, data=0x7cfff5260040) at flow-worker.c:731
#11 0x0000555555f3aa8f in TmThreadsSlotVarRun (tv=0x7d1ff58209c0, p=0x7dcff8d49680, slot=0x7c5ff528ea40) at tm-threads.c:139
#12 0x0000555555f3a153 in TmThreadsSlotProcessPkt (tv=0x7d1ff58209c0, s=0x7c5ff528ea40, p=0x7dcff8d49680)
    at /home/jason/oisf/dev/suricata/review/src/tm-threads.h:201
#13 0x0000555555f3aefa in TmThreadTimeoutLoop (tv=0x7d1ff58209c0, s=0x7c5ff528ea40) at tm-threads.c:190
#14 0x0000555555f3bc10 in SCTmThreadsSlotPacketLoopFinish (tv=0x7d1ff58209c0) at tm-threads.c:285
#15 0x0000555555f3d556 in TmThreadsSlotVar (td=0x7d1ff58209c0) at tm-threads.c:526
#16 0x00007ffff78618d9 in asan_thread_start (arg=0x7bffeb4e4000) at /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_interceptors.cpp:246
#17 0x00007ffff66981b9 in start_thread (arg=<optimized out>) at pthread_create.c:454
#18 0x00007ffff671d21c in __GI___clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78

To repro:

  • Set app-layer.http to detection-only
  • Enable xff

Then run with the attached pcap and rule file -- which GitHub won't let me attach...

@victorjulien victorjulien Jun 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Will check it, but it looks like the behavior was wrong before. Wonder if we're freeing the pstate too early in some cases.

}

void AppLayerParserSetTransactionLogId(AppLayerParserState *pstate, uint64_t tx_id)
{
SCEnter();

if (pstate != NULL)
pstate->log_id = tx_id;

DEBUG_VALIDATE_BUG_ON(pstate == NULL);
pstate->log_id = tx_id;
SCReturn;
}

uint64_t AppLayerParserGetTransactionInspectId(AppLayerParserState *pstate, uint8_t direction)
{
SCEnter();

if (pstate != NULL)
SCReturnCT(pstate->inspect_id[(direction & STREAM_TOSERVER) ? 0 : 1], "uint64_t");

DEBUG_VALIDATE_BUG_ON(1);
SCReturnCT(0ULL, "uint64_t");
DEBUG_VALIDATE_BUG_ON(pstate == NULL);
SCReturnCT(pstate->inspect_id[(direction & STREAM_TOSERVER) ? 0 : 1], "uint64_t");
}

inline uint8_t AppLayerParserGetTxDetectProgress(AppLayerTxData *txd, const uint8_t dir)
Expand Down Expand Up @@ -847,7 +841,6 @@ void AppLayerParserSetTransactionInspectId(const Flow *f, AppLayerParserState *p
if (state_progress < state_done_progress)
break;

/* txd can be NULL for HTTP sessions where the user data alloc failed */
AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx);
const uint8_t inspected_flag = (flags & STREAM_TOSERVER) ? APP_LAYER_TX_INSPECTED_TS
: APP_LAYER_TX_INSPECTED_TC;
Expand Down Expand Up @@ -1108,17 +1101,15 @@ static inline int StateGetProgressCompletionStatus(const AppProto alproto, const
*
* If the stream is disrupted, we return the 'completion' value.
*/
int AppLayerParserGetStateProgress(uint8_t ipproto, AppProto alproto,
void *alstate, uint8_t flags)
int AppLayerParserGetStateProgress(uint8_t ipproto, AppProto alproto, void *tx, uint8_t flags)
{
SCEnter();
int r;
if (unlikely(IS_DISRUPTED(flags))) {
r = StateGetProgressCompletionStatus(alproto, flags);
} else {
uint8_t direction = flags & (STREAM_TOCLIENT | STREAM_TOSERVER);
r = alp_ctx.ctxs[alproto][FlowGetProtoMapping(ipproto)].StateGetProgress(
alstate, direction);
const uint8_t direction = flags & (STREAM_TOCLIENT | STREAM_TOSERVER);
r = alp_ctx.ctxs[alproto][FlowGetProtoMapping(ipproto)].StateGetProgress(tx, direction);
}
SCReturnInt(r);
}
Expand Down Expand Up @@ -1514,21 +1505,18 @@ int AppLayerParserParse(ThreadVars *tv, AppLayerParserThreadCtx *alp_tctx, Flow
if (f->proto == IPPROTO_TCP) {
StreamTcpDisableAppLayer(f);
}
AppLayerParserSetEOF(pstate);
if (pstate != NULL) {
AppLayerParserSetEOF(pstate);
}
SCReturnInt(-1);
}

void AppLayerParserSetEOF(AppLayerParserState *pstate)
{
SCEnter();

if (pstate == NULL)
goto end;

DEBUG_VALIDATE_BUG_ON(pstate == NULL);
SCLogDebug("setting APP_LAYER_PARSER_EOF_TC and APP_LAYER_PARSER_EOF_TS");
SCAppLayerParserStateSetFlag(pstate, (APP_LAYER_PARSER_EOF_TS | APP_LAYER_PARSER_EOF_TC));

end:
SCReturn;
}

Expand All @@ -1537,14 +1525,10 @@ void AppLayerParserSetEOF(AppLayerParserState *pstate)
bool AppLayerParserHasDecoderEvents(AppLayerParserState *pstate)
{
SCEnter();

if (pstate == NULL)
return false;

const AppLayerDecoderEvents *decoder_events = AppLayerParserGetDecoderEvents(pstate);
if (decoder_events && decoder_events->cnt)
return true;

if (pstate != NULL) {
const AppLayerDecoderEvents *decoder_events = AppLayerParserGetDecoderEvents(pstate);
return (decoder_events && decoder_events->cnt);
}
/* if we have reached here, we don't have events */
return false;
}
Expand Down
39 changes: 22 additions & 17 deletions src/detect-file-data.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,34 +70,34 @@ int PrefilterMpmFiledataRegister(DetectEngineCtx *de_ctx, SigGroupHead *sgh, Mpm
typedef struct {
int direction;
AppProto alproto;
uint8_t to_client_progress;
uint8_t to_server_progress;
} DetectFileHandlerProtocol_t;
uint8_t progress_ts;
uint8_t progress_tc;
} DetectFileHandlerProtocol;

/* Table with all filehandler registrations */
DetectFileHandlerTableElmt filehandler_table[DETECT_TBLSIZE_STATIC];

#define ALPROTO_WITHFILES_MAX 16

// file protocols with common file handling
DetectFileHandlerProtocol_t al_protocols[ALPROTO_WITHFILES_MAX] = {
DetectFileHandlerProtocol al_protocols[ALPROTO_WITHFILES_MAX] = {
{ .alproto = ALPROTO_NFS, .direction = SIG_FLAG_TOSERVER | SIG_FLAG_TOCLIENT },
{ .alproto = ALPROTO_SMB, .direction = SIG_FLAG_TOSERVER | SIG_FLAG_TOCLIENT },
{ .alproto = ALPROTO_FTP, .direction = SIG_FLAG_TOSERVER | SIG_FLAG_TOCLIENT },
{ .alproto = ALPROTO_FTPDATA, .direction = SIG_FLAG_TOSERVER | SIG_FLAG_TOCLIENT },
{ .alproto = ALPROTO_HTTP1,
.direction = SIG_FLAG_TOSERVER | SIG_FLAG_TOCLIENT,
.to_client_progress = HTP_RESPONSE_PROGRESS_BODY,
.to_server_progress = HTP_REQUEST_PROGRESS_BODY },
.progress_tc = HTP_RESPONSE_PROGRESS_BODY,
.progress_ts = HTP_REQUEST_PROGRESS_BODY },
{ .alproto = ALPROTO_HTTP2,
.direction = SIG_FLAG_TOSERVER | SIG_FLAG_TOCLIENT,
.to_client_progress = HTTP2ProgData,
.to_server_progress = HTTP2ProgData },
.progress_tc = HTTP2ProgData,
.progress_ts = HTTP2ProgData },
{ .alproto = ALPROTO_SMTP, .direction = SIG_FLAG_TOSERVER }, { .alproto = ALPROTO_UNKNOWN }
};

void DetectFileRegisterProto(
AppProto alproto, int direction, uint8_t to_client_progress, uint8_t to_server_progress)
AppProto alproto, int direction, uint8_t progress_tc, uint8_t progress_ts)
{
size_t i = 0;
while (i < ALPROTO_WITHFILES_MAX && al_protocols[i].alproto != ALPROTO_UNKNOWN) {
Expand All @@ -108,8 +108,8 @@ void DetectFileRegisterProto(
}
al_protocols[i].alproto = alproto;
al_protocols[i].direction = direction;
al_protocols[i].to_client_progress = to_client_progress;
al_protocols[i].to_server_progress = to_server_progress;
al_protocols[i].progress_tc = progress_tc;
al_protocols[i].progress_ts = progress_ts;
if (i + 1 < ALPROTO_WITHFILES_MAX) {
al_protocols[i + 1].alproto = ALPROTO_UNKNOWN;
}
Expand All @@ -127,17 +127,15 @@ void DetectFileRegisterFileProtocols(DetectFileHandlerTableElmt *reg)

if (direction & SIG_FLAG_TOCLIENT) {
DetectAppLayerMpmRegister(reg->name, SIG_FLAG_TOCLIENT, reg->priority, reg->PrefilterFn,
reg->GetData, al_protocols[i].alproto, al_protocols[i].to_client_progress);
NULL, al_protocols[i].alproto, al_protocols[i].progress_tc);
DetectAppLayerInspectEngineRegister(reg->name, al_protocols[i].alproto,
SIG_FLAG_TOCLIENT, al_protocols[i].to_client_progress, reg->Callback,
reg->GetData);
SIG_FLAG_TOCLIENT, al_protocols[i].progress_tc, reg->Callback, NULL);
}
if (direction & SIG_FLAG_TOSERVER) {
DetectAppLayerMpmRegister(reg->name, SIG_FLAG_TOSERVER, reg->priority, reg->PrefilterFn,
reg->GetData, al_protocols[i].alproto, al_protocols[i].to_server_progress);
NULL, al_protocols[i].alproto, al_protocols[i].progress_ts);
DetectAppLayerInspectEngineRegister(reg->name, al_protocols[i].alproto,
SIG_FLAG_TOSERVER, al_protocols[i].to_server_progress, reg->Callback,
reg->GetData);
SIG_FLAG_TOSERVER, al_protocols[i].progress_ts, reg->Callback, NULL);
}
}
}
Expand Down Expand Up @@ -528,6 +526,13 @@ uint8_t DetectEngineInspectFiledata(DetectEngineCtx *de_ctx, DetectEngineThreadC
return DETECT_ENGINE_INSPECT_SIG_NO_MATCH;
}

typedef struct PrefilterMpmFiledata {
int list_id;
int base_list_id;
const MpmCtx *mpm_ctx;
const DetectEngineTransforms *transforms;
} PrefilterMpmFiledata;

/** \brief Filedata Filedata Mpm prefilter callback
*
* \param det_ctx detection engine thread ctx
Expand Down
11 changes: 0 additions & 11 deletions src/detect-file-data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,12 @@ typedef struct DetectFileHandlerTableElmt_ {
int priority;
PrefilterRegisterFunc PrefilterFn;
InspectEngineFuncPtr Callback;
InspectionBufferGetDataPtr GetData;
int al_protocols[MAX_DETECT_ALPROTO_CNT];
int tx_progress;
int progress;
} DetectFileHandlerTableElmt;
void DetectFileRegisterFileProtocols(DetectFileHandlerTableElmt *entry);

/* File registration table */
extern DetectFileHandlerTableElmt filehandler_table[DETECT_TBLSIZE_STATIC];

typedef struct PrefilterMpmFiledata {
int list_id;
int base_list_id;
const MpmCtx *mpm_ctx;
const DetectEngineTransforms *transforms;
} PrefilterMpmFiledata;

uint8_t DetectEngineInspectFiledata(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx,
const DetectEngineAppInspectionEngine *engine, const Signature *s, Flow *f, uint8_t flags,
void *alstate, void *txv, uint64_t tx_id);
Expand Down
13 changes: 9 additions & 4 deletions src/detect-parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1402,12 +1402,12 @@ static int SigParseProto(Signature *s, const char *protostr)

bool has_hook = strchr(proto, ':') != NULL;
if (has_hook) {
char *xsaveptr = NULL;
p = strtok_r(proto, ":", &xsaveptr);
h = strtok_r(NULL, ":", &xsaveptr);
char *rem = NULL;
p = strtok_r(proto, ":", &rem);
h = rem;
SCLogDebug("p: '%s' h: '%s'", p, h);
}
if (p == NULL) {
if (p == NULL || strlen(p) == 0) {
SCLogError("invalid protocol specification '%s'", proto);
return -1;
}
Expand All @@ -1422,6 +1422,11 @@ static int SigParseProto(Signature *s, const char *protostr)
AppLayerProtoDetectSupportedIpprotos(s->alproto, s->init_data->proto.proto);

if (h) {
if (strlen(h) == 0) {
SCLogError("invalid protocol specification '%s'", proto);
return -1;
}

/* FW hook LTE mode */
SCLogDebug("hook '%s'", h);
if (*h == '<') {
Expand Down
3 changes: 1 addition & 2 deletions src/output-json-anomaly.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,7 @@ static int JsonAnomalyTxLogger(ThreadVars *tv, void *thread_data, const Packet *

static inline bool AnomalyHasParserEvents(const Packet *p)
{
return (p->flow && p->flow->alparser &&
AppLayerParserHasDecoderEvents(p->flow->alparser));
return (p->flow && AppLayerParserHasDecoderEvents(p->flow->alparser));
}

static inline bool AnomalyHasPacketAppLayerEvents(const Packet *p)
Expand Down
Loading