Small memory leak in sndfile-spectrogram

[musicinmybrain]

When I run the tests on Fedora 34 x86_64, after applying https://github.com/libsndfile/sndfile-tools/pull/74 and https://github.com/libsndfile/sndfile-tools/pull/75, I still get a leak in sndfile-spectrogram.

==275188== Memcheck, a memory error detector
==275188== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==275188== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==275188== Command: bin/sndfile-spectrogram tmp-20210430T154917/chirp.wav 640 480 tmp-20210430T154917/chirp.png
==275188== 
==275188== 
==275188== HEAP SUMMARY:
==275188==     in use at exit: 841,768 bytes in 10,713 blocks
==275188==   total heap usage: 42,897 allocs, 32,184 frees, 9,186,629 bytes allocated
==275188== 
==275188== 369 (256 direct, 113 indirect) bytes in 1 blocks are definitely lost in loss record 221 of 365
==275188==    at 0x484086F: malloc (vg_replace_malloc.c:380)
==275188==    by 0x51B93D1: FcPatternObjectInsertElt (fcpat.c:545)
==275188==    by 0x51BD3C0: FcPatternObjectAddWithBinding (fcpat.c:732)
==275188==    by 0x51BFFAD: FcPatternDuplicate (fcpat.c:1222)
==275188==    by 0x4BA2184: _cairo_ft_font_face_create_for_pattern (cairo-ft-font.c:3301)
==275188==    by 0x4BA2283: _cairo_ft_font_face_create_for_toy.lto_priv.0 (cairo-ft-font.c:3137)
==275188==    by 0x4B6066D: UnknownInlinedFun (cairo-toy-font-face.c:168)
==275188==    by 0x4B6066D: UnknownInlinedFun (cairo-toy-font-face.c:197)
==275188==    by 0x4B6066D: INT_cairo_toy_font_face_create.part.0 (cairo-toy-font-face.c:321)
==275188==    by 0x4B68433: cairo_select_font_face (cairo.c:3041)
==275188==    by 0x4047EA: render_spect_border.constprop.0 (in /home/ben/src/forks/sndfile-tools/bin/sndfile-spectrogram)
==275188==    by 0x402C22: main (in /home/ben/src/forks/sndfile-tools/bin/sndfile-spectrogram)
==275188== 
==275188== LEAK SUMMARY:
==275188==    definitely lost: 256 bytes in 1 blocks
==275188==    indirectly lost: 113 bytes in 4 blocks
==275188==      possibly lost: 0 bytes in 0 blocks
==275188==    still reachable: 841,399 bytes in 10,708 blocks
==275188==         suppressed: 0 bytes in 0 blocks
==275188== Reachable blocks (those to which a pointer was found) are not shown.
==275188== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==275188== 
==275188== For lists of detected and suppressed errors, rerun with: -s
==275188== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I’m not using any special build flags. (./autogen.sh && ./configure && make -j)

A quick review of spectrogram.c didn’t reveal any obvious problems, nor did a quick skim through the relevant cairo internals.

Adding cairo_debug_reset_static_data() at the end of the main routine does “fix” the leak, although I am not suggesting it as a patch.

I’m reporting this in case someone else readily spots what I missed.

[evpobr]

According to documenation it is false positive:

This function is intended to be useful when using memory-checking tools such as valgrind. When valgrind's memcheck analyzes a cairo-using program without a call to cairo_debug_reset_static_data(), it will report all data reachable via cairo's static objects as "still reachable". Calling cairo_debug_reset_static_data() just prior to program termination will make it easier to get squeaky clean reports from valgrind.

This function is intended to be useful when using memory-checking tools such as valgrind. When valgrind's memcheck analyzes a cairo-using program without a call to cairo_debug_reset_static_data(), it will report all data reachable via cairo's static objects as "still reachable". Calling cairo_debug_reset_static_data() just prior to program termination will make it easier to get squeaky clean reports from valgrind

I think we can safely add it to the end of main().

[musicinmybrain]

The interesting thing is that the documentation talks only about “still reachable” memory, which, sure, Valgrind saw plenty of that, and by default doesn’t complain about it—but the Valgrind/memcheck report is about “definitely lost” and “indirectly lost” memory. It’s possible that adding cairo_debug_reset_static_data() somehow accidentally papers over a real problem rather than just suppressing a false positive.

On the other hand, I’m not convinced the leak is a sndfile-tools problem, and in any case a small-ish memory leak in a one-shot command-line tool is the least concerning kind of memory error compared to other things Valgrind is able to find, like relying on uninitialized memory or overrunning a buffer.

[evpobr]

How do you think, it is better to add cairo_debug_reset_static_data()?

[musicinmybrain]

I wasn’t sure. I’m still not 100% sure, but after further examination, I think that the “leaked” memory is indeed indirectly held by one of cairo’s static objects, and Valgrind can’t see the reference to it because fontconfig uses an integer offset instead of a pointer in FcPattern to reference the associated “elements.”

If I’m right about that, then it is actually a false positive from Valgrind, and adding cairo_debug_reset_static_data() to avoid it is the right thing to do.

[evpobr]

So, pull request? :smile:

[musicinmybrain]

Ok, sure, I’ll “commit to” this solution. :stuck_out_tongue: