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 memory leak on signature->block_sigs #147

Merged
merged 2 commits into from Jun 14, 2018

Conversation

telles-simbiose
Copy link
Contributor

@telles-simbiose telles-simbiose commented Jun 6, 2018

I noticed that there was a memory leak on rs_job_iter, after investigating I found that it was not freeing memory hold by rs_signature_t->block_sigs.

This patch fixed the memory leak.

Copy link
Member

@dbaarda dbaarda left a comment

Wow, that's embarrassing, how did I miss that.

I think what happened is at one point the blocksums were an unsized array at the end of the rs_signature_t instead of a separate allocation, and thus didn't need it's own free.

This is a good catch, but your fix is slightly wrong... you need to do the free before the sig is zeroed, otherwise you are just freeing a null pointer, which is a no-op.

src/sumset.c Outdated
@@ -168,6 +168,8 @@ void rs_signature_done(rs_signature_t *sig)
{
hashtable_free(sig->hashtable);
rs_bzero(sig, sizeof(*sig));

free(sig->block_sigs);
Copy link
Member

@dbaarda dbaarda Jun 8, 2018

Choose a reason for hiding this comment

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

You need to do this before the sig is zero'ed... so put it after hashtable_free() and before rs_bzero().

@telles-simbiose
Copy link
Contributor Author

@telles-simbiose telles-simbiose commented Jun 8, 2018

Done @dbaarda .
It was wrong because I did it in my application and then just moved to sumset.c

@dbaarda dbaarda merged commit dfba898 into librsync:master Jun 14, 2018
1 check passed
@anthonylouisbsb anthonylouisbsb deleted the fixbug/signature-memory-leak branch Sep 1, 2021
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.

None yet

2 participants