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

Discard truncate fn/v3 #11435

Closed
wants to merge 2 commits into from
Closed

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Jul 5, 2024

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

SV_BRANCH=OISF/suricata-verify#1964

Previous PR: #11411

Changes since v2:

  • added explanation in the commit message
  • removed smb truncate fn and calls
  • comments addressed

inashivb added 2 commits July 5, 2024 15:35
as its functionality is already covered by the generic code.
This removes APP_LAYER_PARSER_TRUNC_TC and APP_LAYER_PARSER_TRUNC_TS
flags as well as FlowGetDisruptionFlags sets STREAM_DEPTH flag in case
the respective stream depth was reached. This flag tells that whether
all the open files should be truncated or not.

Bug 7044
truncate fn is only active and used by dcerpc and smb parsers. In case
stream depth is reached for any side, truncate fn is supposed to set the
tx entity (request/response) in the same direction as complete so the
other side is not forever waiting for data.

However, whether the stream depth is reached is already checked by
AppLayerParserGetStateProgress fn which is called by:
- DetectTx
- DetectEngineInspectBufferGeneric
- AppLayerParserSetTransactionInspectId
- OutputTxLog
- AppLayerParserTransactionsCleanup

and, in such a case, StateGetProgressCompletionStatus is returned for
the respective direction. This fn following efc9a7a, always returns 1
as long as the direction is valid meaning that the progress for the
current direction is marked complete. So, there is no need for the additional
callback to mark the entities as done in case of depth or a gap.
Remove all such glue code and callbacks for truncate fns.

Bug 7044
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.44%. Comparing base (eeec609) to head (bb6f014).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11435   +/-   ##
=======================================
  Coverage   82.44%   82.44%           
=======================================
  Files         938      938           
  Lines      248068   247969   -99     
=======================================
- Hits       204513   204441   -72     
+ Misses      43555    43528   -27     
Flag Coverage Δ
fuzzcorpus 60.08% <100.00%> (-0.02%) ⬇️
livemode 18.70% <0.00%> (-0.01%) ⬇️
pcap 43.81% <100.00%> (+0.08%) ⬆️
suricata-verify 61.47% <100.00%> (+0.05%) ⬆️
unittests 59.44% <40.00%> (+<0.01%) ⬆️

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

@inashivb inashivb marked this pull request as ready for review July 5, 2024 11:23
@inashivb inashivb mentioned this pull request Jul 5, 2024
@inashivb inashivb closed this Jul 5, 2024
@inashivb inashivb deleted the discard-truncate-fn/v3 branch July 5, 2024 12:18
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.

2 participants