search

otrv4 / libotr-ng

A new implementation of OTR with support for version 4. This is a mirror of https://bugs.otr.im/otrv4/libotr-ng
Other
43 stars 9 forks source link

potential memory leak in otrng_smp_initiate (currently not actually leaking) #208

Closed osmk closed 5 years ago

osmk commented 5 years ago

Hi, looking at the code below there is a do {...} while(0); construction to retry on certain errors.

https://github.com/otrv4/libotr-ng/blob/master/src/smp.c#L201-L224

The retry logic would repeat the allocation below, overwriting an allocated but not freed pointer with a new one, causing a leak

https://github.com/otrv4/libotr-ng/blob/c6185352b30ef788e9b1eba0f88be2c57ebce9e1/src/smp.c#L208

In practice this doesn't happen, because otrng_smp_message_1_serialize currently never returns anything but a success. It would probably be good to fix this though.

The smallest fix might be to free msg.question before retrying.

Another path could be to remove the retry logic until the errors it protects against are possible. Conceivably, as otrng_smp_message_1_serialize is an internal function and changing the signature of it is fairly cheap, one might consider changing it to not return a value for now, as it should always succeed in its current state.

I'd be happy to make a PR if you'd like

claucece commented 5 years ago

Hey!

Hi, looking at the code below there is a do {...} while(0); construction to retry on certain errors.

Not really. The condition will execute exactly one time. See: http://www.bruceblinn.com/linuxinfo/DoWhile.html

The retry logic would repeat the allocation below, overwriting an allocated but not freed pointer with a new one, causing a leak

No. It will be allocated once. If an error occurs, the code will jump to otrng_smp_message_1_destroy(&msg) which frees the allocated msg.question.

Thanks!

osmk commented 5 years ago

Ah, my bad, I should have tested more before reporting