Skip to content

src/reputation: remove unused code#15256

Closed
spinaev wants to merge 2 commits intoOISF:mainfrom
spinaev:fix-8500
Closed

src/reputation: remove unused code#15256
spinaev wants to merge 2 commits intoOISF:mainfrom
spinaev:fix-8500

Conversation

@spinaev
Copy link
Copy Markdown

@spinaev spinaev commented Apr 24, 2026

Remove useless while() and simplify skiping empty lines/comments

Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Removed useless while() and simplified skiping empty lines/comments

Provide values to any of the below to override the defaults.

  • To use a Suricata-Verify or Suricata-Update pull request,
    link to the pull request in the respective _BRANCH variable.
  • Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=OISF/suricata-verify#3059
SU_REPO=
SU_BRANCH=

@spinaev spinaev requested a review from victorjulien as a code owner April 24, 2026 13:34
@spinaev
Copy link
Copy Markdown
Author

spinaev commented Apr 24, 2026

@catenacyber here it is

Comment thread src/reputation.c
if (line[0] == '\n' || line [0] == '\r' || line[0] == ' ' || line[0] == '#' || line[0] == '\t')
continue;

while (isspace((unsigned char)line[--len]));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we simply just remove this line ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes, we can. i just dont like code duplication...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok : can we split the commit in 2 commits ? I think it is better to have the smallest commit possible to fix the underflow bug, then we can have another commit to deduplicate code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done, i think

@catenacyber
Copy link
Copy Markdown
Contributor

Could we have a Suricata-verify test with reputation file 0xC 0xA ?

Did you try to fuzz this code ?

@github-actions
Copy link
Copy Markdown

NOTE: This PR may contain new authors.

@spinaev
Copy link
Copy Markdown
Author

spinaev commented Apr 29, 2026

@catenacyber if i learn how to make "suricata verify tests" - yes, i can add it.

no, i don't try to fuzz it

@catenacyber
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown

NOTE: This PR may contain new authors.

@catenacyber
Copy link
Copy Markdown
Contributor

CI for C code formatting is red : https://github.com/OISF/suricata/actions/runs/25101263021/job/73551519009?pr=15256

You can fix it with ./scripts/clang-format.sh

@spinaev
Copy link
Copy Markdown
Author

spinaev commented Apr 29, 2026

@catenacyber now i can create a test, but it will not fail, just cause buffer underrun. shall i add such test?

@catenacyber
Copy link
Copy Markdown
Contributor

It should fail when Suricata is built with ASAN, so yes please add such a test

@spinaev
Copy link
Copy Markdown
Author

spinaev commented Apr 29, 2026

It should fail when Suricata is built with ASAN, so yes please add such a test

done

@catenacyber
Copy link
Copy Markdown
Contributor

Thanks. Could you rebase to latest main and to a new PR to get green CI ?
See https://docs.suricata.io/en/latest/devguide/contributing/code-submission-process.html#pull-requests

@spinaev
Copy link
Copy Markdown
Author

spinaev commented Apr 30, 2026

Thanks. Could you rebase to latest main and to a new PR to get green CI ? See https://docs.suricata.io/en/latest/devguide/contributing/code-submission-process.html#pull-requests

#15285

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