Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zero-initialize sfinfo in resample.c to fix Valgrind error #74

Merged
merged 2 commits into from May 1, 2021

Conversation

musicinmybrain
Copy link
Contributor

On Fedora 34 x86_64, the valgrind checks in tests/test_wrapper give:

valgrind bin/sndfile-generate-chirp   : ok
valgrind bin/sndfile-resample         : 1 errors, 0 bytes leaked
valgrind bin/sndfile-resample         : 1 errors, 0 bytes leaked
valgrind bin/sndfile-resample         : 1 errors, 0 bytes leaked
valgrind bin/sndfile-spectrogram      : 0 errors, 256 bytes leaked
valgrind bin/sndfile-waveform         : 0 errors, 392 bytes leaked

This PR zero-initializes the SF_INFO structure, which fixes the sndfile-resample errors, like the following:

==164062== Memcheck, a memory error detector
==164062== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==164062== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==164062== Command: bin/sndfile-resample -to 48000 -c 2 tmp-20210430T120032/chirp.wav tmp-20210430T120032/chirp2.wav
==164062==
==164062== Conditional jump or move depends on uninitialised value(s)
==164062==    at 0x4886E9A: psf_open_file (sndfile.c:2946)
==164062==    by 0x109518: main (resample.c:146)
==164062==
Input File    : tmp-20210430T120032/chirp.wav
Sample Rate   : 44100
Input Frames  : 44100

SRC Ratio     : 1.088435
Converter     : Fastest Sinc Interpolator

Output File   : tmp-20210430T120032/chirp2.wav
Sample Rate   : 48000
 - remaining  :                   0

Output has clipped. Restarting conversion to prevent clipping.

 - remaining  :                   0
Output Frames : 48000

==164062==
==164062== HEAP SUMMARY:
==164062==     in use at exit: 0 bytes in 0 blocks
==164062==   total heap usage: 20 allocs, 20 frees, 150,416 bytes allocated
==164062==
==164062== All heap blocks were freed -- no leaks are possible
==164062==
==164062== Use --track-origins=yes to see where uninitialised values come from
==164062== For lists of detected and suppressed errors, rerun with: -s
==164062== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

The result is:

valgrind bin/sndfile-generate-chirp   : ok
valgrind bin/sndfile-resample         : ok
valgrind bin/sndfile-resample         : ok
valgrind bin/sndfile-resample         : ok
valgrind bin/sndfile-spectrogram      : 0 errors, 256 bytes leaked
valgrind bin/sndfile-waveform         : 0 errors, 392 bytes leaked

@musicinmybrain
Copy link
Contributor Author

Added the same change everywhere else SF_INFO is used.

@evpobr evpobr merged commit 3d91cc9 into libsndfile:master May 1, 2021
@musicinmybrain
Copy link
Contributor Author

It turns out that, for several of these (src/jackplay.c, src/mix-to-mono.c, src/spectrogram.c, src/waveform.c), the zero-initialization was already being done by a memset(), which I should have noticed but didn’t. After this PR, the SF_INFO is (theoretically; the compiler might optimize it out) zero-initialized twice.

To resolve the duplicate initializations, would you prefer a PR to revert the C99-style zero-initialization where memset() is already used, or to remove the memset()s and stick with the style in this PR?

@SoapGentoo
Copy link
Member

Prefer C99-style zero-initialization over memset, always

@musicinmybrain
Copy link
Contributor Author

Prefer C99-style zero-initialization over memset, always

Thanks—me too, but everyone’s different, so I ask!

PR submitted as #81.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants