Qalculate / libqalculate

Qalculate! library and CLI
https://qalculate.github.io/
GNU General Public License v2.0
1.81k stars 146 forks source link

Fix memory leak from mpfr library #680

Closed dschopf closed 2 months ago

dschopf commented 2 months ago

This fixes a memory leak from the mpfr library:

=================================================================
==43801==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 392 byte(s) in 18 object(s) allocated from:
    #0 0x7f14e78fb6e2 in realloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85
    #1 0x7f14e576a958 in __gmp_default_reallocate (/usr/lib/libgmp.so.10+0x11958) (BuildId: 7d4a85294987f7ccba2d4a2e1e9435ed16ca2ae1)

Direct leak of 56 byte(s) in 1 object(s) allocated from:
    #0 0x7f14e78fca31 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f14e576a90d in __gmp_default_allocate (/usr/lib/libgmp.so.10+0x1190d) (BuildId: 7d4a85294987f7ccba2d4a2e1e9435ed16ca2ae1)
    #2 0x7f14e581b5a4  (/usr/lib/libmpfr.so.6+0x1c5a4) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #3 0x7f14e581f270 in mpfr_const_log2_internal (/usr/lib/libmpfr.so.6+0x20270) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #4 0x7f14e5852c14 in mpfr_cache (/usr/lib/libmpfr.so.6+0x53c14) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #5 0x7f14e58208ec in mpfr_log (/usr/lib/libmpfr.so.6+0x218ec) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #6 0x7f14e583fed4 in mpfr_log10 (/usr/lib/libmpfr.so.6+0x40ed4) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #7 0x7f14e706b81f in Number::log(Number const&) /usr/src/debug/libqalculate/libqalculate/libqalculate/Number.cc:7651
    #8 0x100000000  (<unknown module>)

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f14e78fca31 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f14e576a90d in __gmp_default_allocate (/usr/lib/libgmp.so.10+0x1190d) (BuildId: 7d4a85294987f7ccba2d4a2e1e9435ed16ca2ae1)
    #2 0x7f14e58170fa in mpfr_init2 (/usr/lib/libmpfr.so.6+0x180fa) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #3 0x7f14e5852c0b in mpfr_cache (/usr/lib/libmpfr.so.6+0x53c0b) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #4 0x7f14e58208bd in mpfr_log (/usr/lib/libmpfr.so.6+0x218bd) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #5 0x7f14e583fed4 in mpfr_log10 (/usr/lib/libmpfr.so.6+0x40ed4) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #6 0x7f14e706b81f in Number::log(Number const&) /usr/src/debug/libqalculate/libqalculate/libqalculate/Number.cc:7651
    #7 0x100000000  (<unknown module>)

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f14e78fca31 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f14e576a90d in __gmp_default_allocate (/usr/lib/libgmp.so.10+0x1190d) (BuildId: 7d4a85294987f7ccba2d4a2e1e9435ed16ca2ae1)
    #2 0x7f14e58170fa in mpfr_init2 (/usr/lib/libmpfr.so.6+0x180fa) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #3 0x7f14e5852c0b in mpfr_cache (/usr/lib/libmpfr.so.6+0x53c0b) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #4 0x7f14e58208ec in mpfr_log (/usr/lib/libmpfr.so.6+0x218ec) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #5 0x7f14e583fed4 in mpfr_log10 (/usr/lib/libmpfr.so.6+0x40ed4) (BuildId: 4d244e3e86a7787f3b1a0fbe6b56d4afac4c6038)
    #6 0x7f14e706b81f in Number::log(Number const&) /usr/src/debug/libqalculate/libqalculate/libqalculate/Number.cc:7651
    #7 0x100000000  (<unknown module>)

Direct leak of 16 byte(s) in 2 object(s) allocated from:
    #0 0x7f14e78fca31 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f14e576a90d in __gmp_default_allocate (/usr/lib/libgmp.so.10+0x1190d) (BuildId: 7d4a85294987f7ccba2d4a2e1e9435ed16ca2ae1)

SUMMARY: AddressSanitizer: 528 byte(s) leaked in 23 allocation(s).

Not sure if this is the correct fix, especially when the mpfr cache might be used between multiple Numbers.

Calling

    mpfr_free_cache();
    mpfr_free_cache2(MPFR_FREE_LOCAL_CACHE);
    mpfr_free_cache2(MPFR_FREE_GLOBAL_CACHE);
    mpfr_mp_memory_cleanup();

in the Calculator destructor did not fix the issue, I guess this may be because I use the Calculator from a different thread.

hanna-kn commented 2 months ago

Running both mpfr_free_cache() and mpfr_free_cache2() seems superfluous.

It should be enough to run mpfr_free_cache2(MPFR_FREE_LOCAL_CACHE) in the threads used by Calculator (or rather in the Thread class), and in mpfr_free_cache() in the Calculator destructor.

I've added mpfr_free_cache() as described above, and fixed other memory leaks, in commit https://github.com/Qalculate/libqalculate/commit/42c5af5333c527c44b67f2d480680029c5284d49. I will therefore close this pull request.

dschopf commented 2 months ago

I am sorry but your changes don't fix my issue. I still get the same memory leaks as before your commit, 523 bytes from 23 allocations.

Currently I am only able to reproduce this with my own application, I will post again once I have a minimal working example which shows the same behavior.

hanna-kn commented 2 months ago

Have you tried commit https://github.com/Qalculate/libqalculate/commit/48d1b6d5cd6254e447cd20ea98ac31b22ef6b545 and https://github.com/Qalculate/libqalculate/commit/de0e2bf833e5d2c54c9da0447cee68ecd82897e9?

If your application uses multiple threads (not using the libqalculate Thread class), perhaps you should simple call mpfr_free_cache2(MPFR_FREE_LOCAL_CACHE) in each. Maybe a Calculator::freeThreadCaches() function should be added.

The potential leaks, caused by not calling mpfr_free_cache, does seem very small (< 1 kB per thread). I do not know to what extent freeing the caches frequently (as would be the cases if done in the Number destructor) would hurt performance (probably not really noticeable either).

Note that libqalculate is generally not thread-safe.

dschopf commented 2 months ago

I have tested with 0b02550 and there the issue is still there. However, I can fix it as you suggested, by adding mpfr_free_cache2(MPFR_FREE_LOCAL_CACHE) before shutting down my threads. So this issue is resolved.

I would also welcome a Calculcator::freeThreadCaches(), otherwise I also have to link to libmpfr for that single function call.