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.

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

Jump to bottom

tcp: fix 'broken ack' on flow timeout #11463

Closed
wants to merge 1 commit into from
Closed

tcp: fix 'broken ack' on flow timeout #11463

wants to merge 1 commit into from

Conversation

victorjulien
Copy link
Member

Don't set an ACK value if ACK flag is no longer set. This avoids a bogus pkt_broken_ack event set.

Fixes: ebf465a ("tcp: do not assign TCP flags to pseudopackets")

Ticket: #7158.

Replaces #11153
https://redmine.openinfosecfoundation.org/issues/7158

@victorjulien
tcp: fix 'broken ack' on flow timeout
87630b2 
Don't set an ACK value if ACK flag is no longer set. This avoids a bogus
`pkt_broken_ack` event set.

Fixes: ebf465a ("tcp: do not assign TCP flags to pseudopackets")

Ticket: OISF#7158.
@victorjulien victorjulien mentioned this pull request Jul 10, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.55%. Comparing base (090079c) to head (87630b2).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #11463      +/-   ##
==========================================
+ Coverage   82.52%   82.55%   +0.02%     
==========================================
  Files         938      938              
  Lines      248297   248295       -2     
==========================================
+ Hits       204917   204980      +63     
+ Misses      43380    43315      -65
Flag Coverage Δ
fuzzcorpus 60.67% <100.00%> (+0.23%) ⬆️
livemode 18.70% <0.00%> (-0.01%) ⬇️
pcap 43.77% <100.00%> (-0.01%) ⬇️
suricata-verify 61.54% <100.00%> (+<0.01%) ⬆️
unittests 59.43% <0.00%> (+<0.01%) ⬆️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21447

jufajardini
jufajardini approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

This looks good to me. And it covers all places where the pseudo packet could be updated for the ACK, right?

Do we need any dedicated test for cases like this?

@victorjulien victorjulien added this to the 8.0 milestone Jul 11, 2024
jasonish
jasonish approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

If I read the fix of ebf465a correctly, this was more or less missed when ebf465a set the th_flags = 0 instead of TH_ACK.

@victorjulien victorjulien mentioned this pull request Jul 12, 2024
Merged
@victorjulien
Copy link
Member Author

Merged in #11488, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonish jasonish jasonish approved these changes

@jufajardini jufajardini jufajardini approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.
4 participants
@victorjulien @suricata-qa @jasonish @jufajardini