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

Fix potential memory leak in BloomFilterWrapper #833

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

s3cur3
Copy link
Contributor

@s3cur3 s3cur3 commented Feb 14, 2021

Description:

As best I can tell, this is mostly theoretical
at this point---the BloomFilterWrapper is currently
retained for the lifetime of the program. If this
ever were not the case, though, we'd be leaking
the allocated BloomFilter object without a delete
corresponding to its new.

Steps to test this PR:

You can observe the leak in the test suite, which
does create temporary BloomFilterWrappers: just
add a destructor to BloomFilter.hpp in the submodule
like this:

~BloomFilter() { printf("Destroyed Bloom\n"); }

...and observe that it's never called without this
patch, and consistently called with it.

Issue URL: #806
Internal URL: https://app.asana.com/0/392891325557410/1199935106721785

As best I can tell, this is mostly theoretical
at this point---the BloomFilterWrapper is currently
retained for the lifetime of the program. If this
ever were *not* the case, though, we'd be leaking
the allocated BloomFilter object without a `delete`
corresponding to its `new`.

You can observe the leak in the test suite, which
*does* create temporary `BloomFilterWrapper`s: just
add a destructor to BloomFilter.hpp in the submodule
like this:

    ~BloomFilter() { printf("Destroyed Bloom\n"); }

...and observe that it's never called without this
patch, and consistently called with it.
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

Thanks for this! Confirmed this works by using a breakpoint on the unit tests. Quick question, for completeness is it worth making the filter pointer NULL or is that overkill?

@s3cur3
Copy link
Contributor Author

s3cur3 commented Feb 16, 2021

Not necessary, actually—the C++ spec (and thus Obj-C++ as well) specifies that deleting a null pointer is a no-op. Relevant StackOverflow discussion here.

@brindy
Copy link
Contributor

brindy commented Feb 16, 2021

Oh, I meant making filter NULL after deleting it. I guess it's low risk though as dealloc() would have to be called directly, and that would be madness. Thanks again! Will merge now.

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

Thanks again!

@brindy brindy merged commit 9639784 into duckduckgo:develop Feb 16, 2021
samsymons added a commit that referenced this pull request Feb 17, 2021
* develop:
  Remove experiment code (#834)
  Fix potential memory leak in BloomFilterWrapper (#833)
@brindy
Copy link
Contributor

brindy commented Feb 23, 2021

Just wanted to note that this did indeed leak under the right conditions. Specifically when there was data to update from the remote endpoint, the bloom filter is instanciated/replaced so the old one would be hanging around. So thanks again for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants