Skip to content

Detect tcp noupdatetotx 6299 v8#10127

Closed
catenacyber wants to merge 4 commits intoOISF:masterfrom
catenacyber:detect-tcp-noupdatetotx-6299-v8
Closed

Detect tcp noupdatetotx 6299 v8#10127
catenacyber wants to merge 4 commits intoOISF:masterfrom
catenacyber:detect-tcp-noupdatetotx-6299-v8

Conversation

@catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6299

Describe changes:

  • Optimization to not run transaction detection when a TCP packet did not update anything (no call to AppLayerParserParse) in the packet direction
  • Merge sorted lists should be faster than qsort
  • do not store state if there is no flags set
  • cleans mqtt code for setting events

#9638 rebased

ping @victorjulien ;-)

For TCP, app_update_direction is set by AppLayerHandleTCPData

Need to move up the call to AppLayerParserSetTransactionInspectId
so that it is only run after DetectRunTx is run

Ticket: 6299
expecially sets transactions to complete when we get a response
without having seen the request.

Ticket: OISF#6299
@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW2_autofp_stats_chk
.uptime 181 192 106.08%

Pipeline 17370

@victorjulien
Copy link
Member

"mqtt: review logic for setting event" <- weird subject. Subject should describe code change.

@@ -152,13 +152,24 @@ static void DetectRun(ThreadVars *th_v,
DetectRunFrames(th_v, de_ctx, det_ctx, p, pflow, &scratch);
Copy link
Member

Choose a reason for hiding this comment

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

commit message is very unclear, not understanding what we're doing and why

}
}
do_sort = (array_idx > x); // sort if match added anything
uint32_t k = array_idx;
Copy link
Member

Choose a reason for hiding this comment

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

can you move this into a static inline helper func? Code is getting too large here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -1378,21 +1378,50 @@ static void DetectRunTx(ThreadVars *tv,

Copy link
Member

Choose a reason for hiding this comment

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

commit message should explain why & how

@@ -1226,7 +1226,7 @@ static bool DetectRunTxInspectRule(ThreadVars *tv,
} else if ((inspect_flags & DE_STATE_FLAG_FULL_INSPECT) == 0 && mpm_in_progress) {
TRACE_SID_TXS(s->id, tx, "no need to store no-match sig, "
"mpm will revisit it");
Copy link
Member

Choose a reason for hiding this comment

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

commit message not explaining why/how and problem this solves

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

Need a lot better commit messages, these don't explain much

@catenacyber
Copy link
Contributor Author

Need a lot better commit messages, these don't explain much

Improving all of them, even if GitHub does not show which comment you put for which commit ;-)

@catenacyber
Copy link
Contributor Author

Continued in #10145

@catenacyber catenacyber closed this Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants