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

radix: Don't leak memory when key can't be added #9273

Closed
wants to merge 5 commits into from

Conversation

jlucovsky
Copy link
Contributor

Continuation of #9264

Thanks to @glongo for the contributions included in the redmine ticket.

Link to redmine ticket: 5748

Describe changes:

  • Check for duplicate keys before adding to radix tree.
  • Free key memory when key is a duplicate

Updates:

  • Rebased this and sv-branch

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

To use a pull request use a branch name like pr/N where N is the pull request number.

SV_REPO=
SV_BRANCH=pr/1190
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

Ensure that memory for the reputation key is released on failed adds.

Contributed by Giuseppe Longo <giuseppe@glongo.it>

Issue: 5748
This commit converts usages of `int` to `bool` within the radix utility
code.
Issue: 5748

This commit adds SC_EEXIST to be used for cases where an
item/resource/artifact with the same attributes already exists.
This commit prevents duplicate IPV4/IPV6 netblocks from being added to the
radix tree.

Contributed by Giuseppe Longo <giuseppe@glongo.it>

Issue: 5748
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #9273 (483d920) into master (9a33c53) will decrease coverage by 3.79%.
The diff coverage is 44.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9273      +/-   ##
==========================================
- Coverage   82.42%   78.64%   -3.79%     
==========================================
  Files         968      968              
  Lines      273952   273797     -155     
==========================================
- Hits       225807   215319   -10488     
- Misses      48145    58478   +10333     
Flag Coverage Δ
fuzzcorpus 64.70% <22.85%> (+0.01%) ⬆️
suricata-verify ?
unittests 62.93% <44.28%> (-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 15355

@@ -484,11 +482,13 @@ void SCRadixReleaseRadixTree(SCRadixTree *tree)
* this key
* \param netmask The netmask (cidr) if we are adding an IP netblock; 255
* if we are not adding an IP netblock
* \param exclusive True (false) if the node should added if not present (present).
Copy link
Member

Choose a reason for hiding this comment

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

I have a bit of trouble parsing this... what is True (false) and present (present)?

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'll reword this and will repost.

src/util-radix-tree.c Show resolved Hide resolved
src/util-radix-tree.c Show resolved Hide resolved
src/util-radix-tree.c Show resolved Hide resolved

memset(tmp_stream, 0, 255);
// memset(tmp_stream, 0, 255);
Copy link
Member

Choose a reason for hiding this comment

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

please remove

@jlucovsky
Copy link
Contributor Author

Continued in #9280

@jlucovsky jlucovsky closed this Jul 24, 2023
@jlucovsky jlucovsky deleted the 5748/9 branch September 5, 2023 14:09
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