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

Memory leak when trying to restart the ZRTP #27

Open
sahara108 opened this issue Oct 22, 2015 · 7 comments
Open

Memory leak when trying to restart the ZRTP #27

sahara108 opened this issue Oct 22, 2015 · 7 comments

Comments

@sahara108
Copy link

screen shot 2015-10-22 at 11 28 12 am

@wernerd
Copy link
Owner

wernerd commented Oct 22, 2015

I cannot confirm this. In the shown function the sequence is:

...
if (algorithm == SrtpEncryptionAESCM || algorithm == SrtpEncryptionAESF8) {
AESencrypt *saAes = new AESencrypt();
if (keyLength == 16)
saAes->key128(k);
else
saAes->key256(k);
key = saAes;
}
else if (algorithm == SrtpEncryptionTWOCM || algorithm == SrtpEncryptionTWOF8) {
if (!twoFishInit) {
Twofish_initialise();
twoFishInit = 1;
}
key = new uint8_t[sizeof(Twofish_key)];
...

As you can see the instance variable 'key' gets the pointer of the 'new' operator.
The destructor deletes this if it's set. 'key' is a void* to cover the pointers
to the different crypto algorithm data structures. Other functions cast the void*
to the correct pointer based on algorithm information.

Werner

Am 22.10.2015 um 06:30 schrieb sahara108:

screen shot 2015-10-22 at 11 28 12 am https://cloud.githubusercontent.com/assets/1707951/10657046/423518e2-78b0-11e5-95d3-cb4c20ed3017.png


Reply to this email directly or view it on GitHub #27.

Werner Dittmann
email: [email protected]
cell: +49 173 44 37 659
PGP key: 82EF5E8B

@sahara108
Copy link
Author

May be it relate to other function, let me provide full log:
screen shot 2015-10-23 at 2 11 07 pm

@sahara108
Copy link
Author

And this one is totally different:
screen shot 2015-10-23 at 2 09 31 pm

@wernerd
Copy link
Owner

wernerd commented Oct 23, 2015

This is similar to your first report. The destructor takes care of deallocation.

Similar to the bigbytes code as I can see this. It's the bnEnd function that takes
carte of this. Please refer how the inline doc of the bnlib.

Werner

Am 23.10.2015 um 09:13 schrieb sahara108:

May be it relate to other function, let me provide full log:
screen shot 2015-10-23 at 2 11 07 pm https://cloud.githubusercontent.com/assets/1707951/10686629/2bc63790-7990-11e5-98fb-55f140ab44be.png


Reply to this email directly or view it on GitHub #27 (comment).

Werner Dittmann
email: [email protected]
cell: +49 173 44 37 659
PGP key: 82EF5E8B

@wernerd
Copy link
Owner

wernerd commented Oct 23, 2015

This one is a bit more complex :-)

When adding algorithms to the configuration (standard or mandatory) then the
vectors in ZrtpConfigure class hold pointers to static data. This static data
describes all available algorithms and this data does not change.

The ZrtpConfigure::clear function clears the vectors and the store pointers.
Don't user "delete" for the pointer because they point to static data which
must not be deallocated after it was initialized.

Werner

Am 23.10.2015 um 09:14 schrieb sahara108:

And this one is totally different:
screen shot 2015-10-23 at 2 09 31 pm https://cloud.githubusercontent.com/assets/1707951/10686644/4a8b4058-7990-11e5-8892-10531f9ad131.png


Reply to this email directly or view it on GitHub #27 (comment).

Werner Dittmann
email: [email protected]
cell: +49 173 44 37 659
PGP key: 82EF5E8B

@sahara108
Copy link
Author

Ok, I will investigate more about the first one. For the second one, did you mean the zrtp_DestroyWrapper will take care of the deallocation.
Here is how I stop zrtp engine:

zrtp_stopZrtpEngine(fabZrtp->zrtpCtx);
zrtp_DestroyWrapper(fabZrtp->zrtpCtx);

Do I need any extra step?

@wernerd
Copy link
Owner

wernerd commented Oct 25, 2015

Am 23.10.2015 um 11:05 schrieb sahara108:

Ok, I will investigate more about the first one. For the second one, did you mean the zrtp_DestroyWrapper will take care of the deallocation.
Here is how I stop zrtp engine:

zrtp_stopZrtpEngine(fabZrtp->zrtpCtx);
zrtp_DestroyWrapper(fabZrtp->zrtpCtx);

Yes, this should do it. Destroy wrapper deletes the allocated objects and their
destructors should ;-) clean up memory that the objects allocated during thier
runtime.

Actually, some other parties and projects did memory leakage tests, statically as well
as dynamically, no real problems were found so far. Nevertheless, each new check may
bring up a yet unseen problem and I'm always happy to learn about this and fix it.

Werner

Do I need any extra step?


Reply to this email directly or view it on GitHub #27 (comment).

Werner Dittmann
email: [email protected]
cell: +49 173 44 37 659
PGP key: 82EF5E8B

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

No branches or pull requests

2 participants