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

InitializeDefaultRepeatedFields() allocates memory but does not release ... #43

Merged
merged 2 commits into from
Oct 9, 2014

Conversation

abuszta
Copy link
Contributor

@abuszta abuszta commented Oct 8, 2014

Using protobuf-2.6.0 and current master results in C++ example leakage in src/google/protobuf/extension_set.cc line 1632 and further.

This problem reproduces with current protobuf example:

valgrind --tool=memcheck --leak-check=full --show-leak-kinds=all ./list_people_cpp database

Sample output:

...
==32205== 16 bytes in 1 blocks are still reachable in loss record 3 of 10
==32205==    at 0x4C29180: operator new(unsigned long) (vg_replace_malloc.c:324)
==32205==    by 0x4E8D146: google::protobuf::internal::InitializeDefaultRepeatedFields() (in /usr/lib/x86_64-linux-gnu/libprotobuf.so.9.0.0)
==32205==    by 0x400E9F9: call_init.part.0 (dl-init.c:78)
==32205==    by 0x400EAE2: call_init (dl-init.c:36)
==32205==    by 0x400EAE2: _dl_init (dl-init.c:126)
==32205==    by 0x40011C9: ??? (in /lib/x86_64-linux-gnu/ld-2.19.so)
...

It would be nice to have protobuf not leaving this memory unallocated to make good old memory sanitizer happy :).

One of the solutions would be creating destructor for StaticDefaultRepeatedFieldsInitializer which would clean everything. Another solution could be using unique pointers.

@@ -1622,6 +1622,9 @@ struct StaticDefaultRepeatedFieldsInitializer {
StaticDefaultRepeatedFieldsInitializer() {
InitializeDefaultRepeatedFields();
}
~StaticDefaultRepeatedFieldsInitializer() {
DestroyDefaultRepeatedFields();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you register this function with google::protobuf::internal::OnShutdown() instead? ShutdownProtobufLibrary() wil call these callbacks and release all memory allocated by protobuf runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Oct 8, 2014

Please sign this Google CLA:
https://developers.google.com/open-source/cla/individual?csw=1

Thanks.

@xfxyjwf xfxyjwf added this to the Release 2.6.1 milestone Oct 9, 2014
@abuszta
Copy link
Contributor Author

abuszta commented Oct 9, 2014

Signed.

xfxyjwf added a commit that referenced this pull request Oct 9, 2014
Release objects allocated by InitializeDefaultRepeatedFields()
@xfxyjwf xfxyjwf merged commit 5452bd6 into protocolbuffers:master Oct 9, 2014
@abuszta abuszta deleted the bugfix branch October 9, 2014 18:30
TeBoring pushed a commit to TeBoring/protobuf that referenced this pull request Jan 19, 2019
Add flag to MessageDef for whether fields have presence.
bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023
Add flag to MessageDef for whether fields have presence.
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