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

Warnint util 4516 v2 #6792

Closed
wants to merge 2 commits into from
Closed

Warnint util 4516 v2 #6792

wants to merge 2 commits into from

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • Fix integer warnings -Wimplicit-int-conversion in all util files

Modifies #6791

  • also removes warnings from unit tests
  • fixes radix tree loop (using uint, we can no longer have condition >= 0)

@catenacyber catenacyber requested a review from a team as a code owner January 14, 2022 12:20
@catenacyber catenacyber mentioned this pull request Jan 14, 2022
Closed
@codecov
Copy link

codecov bot commented Jan 14, 2022 •
edited
Loading

Codecov Report

Merging #6792 (1057399) into master (78f5e08) will increase coverage by 0.00%.
The diff coverage is 91.46%.

@@           Coverage Diff           @@
##           master    #6792   +/-   ##
=======================================
  Coverage   77.19%   77.19%           
=======================================
  Files         615      615           
  Lines      185624   185617    -7     
=======================================
- Hits       143285   143283    -2     
+ Misses      42339    42334    -5
Flag Coverage Δ
fuzzcorpus 53.17% <45.45%> (-0.02%) ⬇️
suricata-verify 52.69% <48.61%> (-0.15%) ⬇️
unittests 63.08% <89.37%> (+<0.01%) ⬆️

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

victorjulien
victorjulien reviewed Jan 14, 2022
View reviewed changes
src/util-decode-mime.c
// TODO rewrite all this to have app-layer events
find_len = UINT16_MAX;
}
return BasicSearchNocase(src, len, find, (uint16_t)find_len);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just update BasicSearchNocase to accept uint32_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

len is uint32_t(haystack)
The needle is uint16_t
That looks reasonable...

Copy link
Member

Choose a reason for hiding this comment

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

ok, makes sense. Can you add a ticket for tracking the events setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or should this mime code get rustified first ?

Copy link
Member

Choose a reason for hiding this comment

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

For master that would be nice. Would we need a backport for this int handling though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know.
It will be a big work I guess...
I am not sure it is worth it...

@suricata-qa
Copy link

Information:

field test baseline %
tlpr1_stats_chk
.flow.memuse 524447808 553066368 94.83%

Pipeline 5699

This was referenced Jan 18, 2022
Closed
Closed
@victorjulien
Copy link
Member

replaced by #6820

@catenacyber catenacyber mentioned this pull request Feb 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victorjulien victorjulien victorjulien left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@catenacyber @suricata-qa @victorjulien