-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix parsing ip-reputation data with carriage return #9357
Conversation
NOTE: This PR may contain new authors:
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9357 +/- ##
==========================================
- Coverage 82.33% 82.18% -0.16%
==========================================
Files 968 968
Lines 274136 274136
==========================================
- Hits 225716 225293 -423
- Misses 48420 48843 +423
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Thomas, thanks for your fix to this! I see that the SV test created to showcase the issue passes with this PR, so I just want to point out some nit-adjustments :P
- Could you add the ticket number to your commit message? 2786ccb exemplifies how.
- similarly, to comply with our commit styles, can you adjust the commit title to be something like:
iprep: fix parsing ip-rep data with carriage return
This adds the module you've worked on but still keeps message within characters limits :P
- The formatting failure should be easily fixable with clang, let us know if this is unclear to you, please.
NOTE: This PR may contain new authors:
|
The suricata-verifry failures look to be caused by the "tests: files update for nocase fix" commit and not related to these changes. |
Tested things locally, failing tests pass with rebasing SV and Suri. Other requested changes (commit message, clang failure have been addressed). |
Thank you @thomasjwinter for your contribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but needs to be rebased in a new PR to get CI green
replaced by #9426 |
Commit e7c0f0a removed uses of atoi with a new number parsing functions. This broke parsing ip-reputation data files that contained trailing carriage returns as it was being included in the number string to convert.
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6243
SV_BRANCH=OISF/suricata-verify#1343