Skip to content
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.

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

app-layer: update flow counter if an alproto is detected #12454

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Jan 23, 2025

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

Previous PR: #12415

Changes since v1:

  • entire patch is different. No longer tracking to hack into the flow counters

If alproto for the current direction was not detected but the opposite
side was successfully detected, if the Pattern Matching and Pattern
Probing on the flow was also successfully done and the current
direction's alproto is still unknown, a decoder event is set to indicate
that the protocol detection only happened in one direction.

This event is set after having sent the current data to the applayer
parser. Now, the respective applayer parser may or may not successfully
parse the data. However, the alproto on flow is already set from the
other direction so there will be a flow event generated by Suricata. In
order to keep this consistent with the stats, also make sure to
increment the flow counter when the decode event is set so that the flow
counter is incremented irrespective of the parsing status reported by
the applayer parser.

This patch makes stats for several specific applayer flow count equal to
the number of flow events logged for those specific applayer protocols.

Bug 7238
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.63%. Comparing base (95e8427) to head (9215af4).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12454   +/-   ##
=======================================
  Coverage   80.63%   80.63%           
=======================================
  Files         920      920           
  Lines      258704   258704           
=======================================
+ Hits       208595   208603    +8     
+ Misses      50109    50101    -8     
Flag Coverage Δ
fuzzcorpus 56.83% <100.00%> (+0.02%) ⬆️
livemode 19.40% <0.00%> (+<0.01%) ⬆️
pcap 44.29% <100.00%> (-0.05%) ⬇️
suricata-verify 63.24% <100.00%> (-0.02%) ⬇️
unittests 58.51% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@inashivb inashivb requested a review from catenacyber January 23, 2025 07:03
@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.app_layer.flow.tls 624433 645259 103.34%
.app_layer.flow.dcerpc_tcp 43 7339 17067.44%
.app_layer.flow.sip_tcp 0 2 -

Pipeline 24327

@victorjulien
Copy link
Member

Passed my QA. Ran this PR with SV master. Local pipeline 5212, run 697.

@catenacyber
Copy link
Contributor

Do you have SV tests that get improved and can be tested ?

@inashivb
Copy link
Member Author

Do you have SV tests that get improved and can be tested ?

Sorry not seeing any changes in public s-v tests :(

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.

4 participants