search

PromyLOPh / pianobar

Console-based pandora.com player
http://6xq.net/pianobar/
Other
1.74k stars 321 forks source link

Possible memory leak in player.c:openStream() #696

Closed ijustlovemath closed 4 years ago

ijustlovemath commented 4 years ago

Subject of the issue

While researching #695, I came across this snippet of code:

https://github.com/PromyLOPh/pianobar/blob/master/src/player.c#L202

This clearly smells of some behind the scenes memory allocation, and should be freed accordingly using av_dict_free():

https://libav.org/documentation/doxygen/master/group__lavu__dict.html#ga1bafd682b1fbb90e48a4cc3814b820f7

A similar bug looks apparent in player->fctx, but that may be cleaned up elsewhere.

Stack allocated variables with heap data should definitely be cleaned up by softfail if it's necessary (or using some simple goto error handling).

I can add a fix to this to the list of patches I contribute if you like.

PromyLOPh commented 4 years ago

avformat_open_input takes ownership of the dict: https://ffmpeg.org/doxygen/2.8/group__lavf__decoding.html#ga10a404346c646e4ab58f4ed798baca32

fctx is free’d in finish(), if that’s what you mean.

edit: The code is not easy to read, but as far as I know, leak-free for most cases. I’ve checked it using valgrind.

ijustlovemath commented 4 years ago

Okay! Good to know. I'll leave a comment with an explanation and a link to this discussion in a PR.