Menu

#276 Potential memory leaks in mpg123 and out123

svn
closed-fixed
nobody
None
5
2020-05-16
2019-02-25
No

Hi,
I am using the mpg123 and out123 DLLs in Delphi and Free Pascal (did my own translation of the headers), and I have noticed that some functions are returning memory they have allocated, but responsibility of freeing this memory is on the caller. This may be fine for static linking, but when linked dynamically, not to mention by a program written in different language, there is absolutely no way of freeing this memory (pascal programs are usually compiled with internal memory manager, and creating hacks to see what implementation of free() function in which library should be called is kind of meh).
So far, i have found this problem in following functions:

mpg123:mpg123_icy2utf8
out123:out123_drivers
out123:out123_formats
out123:out123_enc_list

I think it should be enough to just export simple wrapper for proper free (eg. mpg123_free(), out123_free()) accepting any pointer.

Discussion

  • Thomas Orgis

    Thomas Orgis - 2019-02-27

    I see your point. Yes, when thinking about such wrapped environments where even different C runtimes can be at play, a mpg123_free() can be useful.

     
  • Thomas Orgis

    Thomas Orgis - 2019-07-23

    I added those wrappers now for mpg123_free() and out123_free(). It's the same free() underneath, but it's two separate libraries and APIs. You could use them from differing builds.

    This will be part of the 1.26 release series.

    Do you have a suggestion for the included ports/mpg123_.pas ? Should we update it (help welcome) or rather drop?

     
  • fred vs

    fred vs - 2019-08-25

    Do you have a suggestion for the included ports/mpg123_.pas ?

    There is a Free Pascal header with dynamic loading of mpg123 library here:

    https://github.com/fredvs/uos/blob/master/src/uos_mpg123.pas

    Fre;D

     
  • František Milt

    František Milt - 2019-10-11

    Sorry for late response, but this is fastest i could respond (it will not be better in the future, sadly).
    My version of the headers can be found here https://github.com/TheLazyTomcat/bnd.mpg123
    At this point, it is only for windows and for older version of the library, but I am planning to add support at least for linux. It is still WIP.

    I have checked the header fred posted, and I have found interesting, and potentially problematic, thing - field alignment in records (structures). The uos_mpg123.pas unit defines C-compatible record packing (= alignment of fields), but then all individual records are declared as packed, meaning the fields are NOT aligned at all (no padding).
    From what i have seen, this is wrong (or is uos using their own, diffrent, DLLs?). I am not good at C, but i have not found anything suggesting the structures used by mpg123 are packed, they have default fields alignment. When trying the "official" DLLs, the structures returned are also normally aligned, not packed. When i have tried using packed records, it immediatelly resulted in AV or the fields had nonsensical content (the padding of course).

    As for pascal unit distributed with mpg123, i think it should stay unless it will be replaced by a better one, but should be marked as obsolete and problematic - records there are declared as packed, see above.

    Dunno if i should create new ticket for this, probably not, because it is not an error on the part of mpg123. But to me, it seems interesting that several pascal translations are using packed records.

    Nevertheless, I have found another problem - out123 should suppress error dialog when loading DLL on windows (at least sometimes). I will create new ticket and give more info there.

     

    Last edit: František Milt 2019-10-11
  • fred vs

    fred vs - 2019-10-11

    Hello František Milt

    From what i have seen, this is wrong (or is uos using their own, diffrent, DLLs?).

    uos gives and uses, for Windows, the binaries DLL's provided by libmpg123.
    For Linux and FreeBSD, they are compiled with clang using the last source of libmpg123.

    https://github.com/fredvs/uos/tree/master/examples/lib/

    Of course if you see something wrong in uos_mpg123.pas, for example for field alignment in records, you are more than welcome to note it in https://github.com/fredvs/uos/issues

    But, till now and afer many, many tests, I did not find any problems using that header.

    Thanks.

    Fre;D

     
  • František Milt

    František Milt - 2019-10-12

    Thanks for response fred vs!
    I have looked deeper into this - most of the records are not affected by different alignment setting (it is given by their layout), but i know for sure that it affects mpg123_text and mpg123_id3v2, and maybe more (haven't tried yet). Just try to read some ID3v2, you'll see.

    I have not tried uos for now, because i primarilly develop in delphi and it is written for FPC, but i will give it a try and if I find the same problem, I will certainly create a new issue at github.

     
  • Thomas Orgis

    Thomas Orgis - 2019-10-12

    So you all seem to aggree that the shipped Pascal port is not needed. I'm fine with that … it's code that I don't maintain. So I guess I'll drop it from the 1.26 release.

     
  • fred vs

    fred vs - 2019-10-12

    @ František Milt :

    but i know for sure that it affects mpg123_text and mpg123_id3v2,

    Well spotted, indeed I remember now that I had problems with id3, so much that in uos I did create a own method to read the id3 tags (and not use the id-methods of mpg123).
    So maybe you have the solution/fixes.

    @Thomas:

    So I guess I'll drop it from the 1.26 release.

    Yes, you can see that the shipped Pascal header is not unanimous.

    Fre;D

     
  • František Milt

    František Milt - 2019-10-12

    Yep, i have just tried uos and indeed, the problem is in the use of packed. When removed, everything works just fine. I have also noticed there are some functions missing (mpg123_meta_free, maybe more, haven't checked). I will write the issue.

    @Thomas Orgis:
    Agree, you can remove it.

     

    Last edit: František Milt 2019-10-12
  • Thomas Orgis

    Thomas Orgis - 2020-01-26

    Removed the outdated header from trunk.

     
  • František Milt

    František Milt - 2020-01-31

    I think this can be closed as resolved, thanks!

     
  • Thomas Orgis

    Thomas Orgis - 2020-05-16
    • status: open --> closed-fixed
     

Log in to post a comment.