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

Coverity fixes v2 #7138

Closed
wants to merge 4 commits into from
Closed

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
None, not sure it deserves one

Describe changes: patches to make coverity happy

  • Prevents a double unlock (by locking again)
  • Makes static code analyzer about a memory leak in xbits parsing (removing dead code for default switch cases)
  • Asserts all cases are covered in debuglog (PKT_IS_IPV6 or PKT_IS_IPV4)
  • fix possible null dereference in debug validation in PacketPoolDestroy

After this and #7131 (which I think is a real bug this one), there will be no more outstanding issues reported by coverity

Replaces #7132 with review taken into account + new bug found by coverity meanwhile

Even if the code seems unreachable for now
Coverity ID: 1502953

As we check just on the next line my_pool against NULL, we should
not dereference it, even for debug validation
@catenacyber catenacyber requested a review from a team as a code owner March 15, 2022 20:01
@catenacyber catenacyber mentioned this pull request Mar 15, 2022
@suricata-qa
Copy link

Information:

field test baseline %
tlpr1_stats_chk
.flow.memuse 491435968 519491328 94.6%

Pipeline 6573

@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #7138 (6b388b8) into master (3a490fb) will increase coverage by 0.08%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #7138      +/-   ##
==========================================
+ Coverage   78.06%   78.15%   +0.08%     
==========================================
  Files         628      628              
  Lines      185266   185268       +2     
==========================================
+ Hits       144635   144787     +152     
+ Misses      40631    40481     -150     
Flag Coverage Δ
fuzzcorpus 60.26% <50.00%> (+0.27%) ⬆️
suricata-verify 54.59% <100.00%> (-0.01%) ⬇️
unittests 63.12% <0.00%> (+<0.01%) ⬆️

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

@catenacyber
Copy link
Contributor Author

CI failure seems unrelated

@@ -813,6 +813,9 @@ Flow *FlowGetFlowFromHash(ThreadVars *tv, FlowLookupStruct *fls,
if (f->use_cnt == 0) {
if (prev_f == NULL) /* if we have no prev it means new_f is now our prev */
prev_f = new_f;
// f got unlocked by TcpReuseReplace and will be unlocked again by
// MoveToWorkQueue
FromHashLockCMP(f);
Copy link
Member

Choose a reason for hiding this comment

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

I think I prefer not unlocking the old flow in TcpReuseReplace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right

@catenacyber catenacyber mentioned this pull request Mar 24, 2022
@catenacyber
Copy link
Contributor Author

Replaced by #7169

This was referenced Mar 25, 2022
@victorjulien
Copy link
Member

Merged all but the flow commit in #7172, thanks!

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