Skip to content

detect: do not run tx detection on non established packets#10307

Closed
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:tcp-skip-not-established-v3
Closed

detect: do not run tx detection on non established packets#10307
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:tcp-skip-not-established-v3

Conversation

@catenacyber
Copy link
Contributor

Follows commit 2fb5059, concurrent against #10286

Link to redmine ticket:
Should there be one specific for this ?

Describe changes:

  • stream/tcp: flag established even on error (error being Suricata StreamTcpStateDispatch returning an error value)

SV_BRANCH=OISF/suricata-verify#1623

#10304 more complete optimization

}
} else if (p->proto == IPPROTO_TCP && p->flow->protoctx) {
FramesPrune(p->flow, p);
if (p->flags & PKT_STREAM_EST) {
Copy link
Member

Choose a reason for hiding this comment

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

should we still call StreamTcpPruneSession in this case? Seems not. Why would an error packet have any effect on state?

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

@victorjulien victorjulien marked this pull request as draft February 3, 2024 09:41
@victorjulien
Copy link
Member

Setting to draft as it does not pass SV.

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW1_stats_chk
.flow.spare 999831 1100740 110.09%
SURI_TLPR1_stats_chk
.memcap_pressure 57 54 94.74%

Pipeline 18069

@catenacyber
Copy link
Contributor Author

Setting to draft as it does not pass SV.

This is only because SV test bug-4903-04 enforces a pcap_cnt value of an invalid TCP packet, and now we match after that.

Why is this RST packet treated as invalid by StreamTcpStateDispatch is another question...

@catenacyber
Copy link
Contributor Author

Replaced by #10322

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