MSAN: Memory leak in wavpack.c

[RootUp]

Summary

While fuzzing WavPack master branch a memory leak was observed in wavpack.c. I've used clang and MSAN to compile the binary.

Vulnerable code

#else
        else if (output_spec) {
            outfilename = malloc (strlen (*argv) + PATH_MAX);
            strcpy (outfilename, *argv);
            output_spec = 0;
        }
        else {
            matches = realloc (matches, (num_files + 1) * sizeof (*matches));
            matches [num_files] = malloc (strlen (*argv) + 10);
            strcpy (matches [num_files], *argv);

            if (*(matches [num_files]) != '-' && *(matches [num_files]) != '@' &&
                !filespec_ext (matches [num_files]))
                    strcat (matches [num_files], (config.qmode & QMODE_RAW_PCM) ? ".raw" : ".wav");

            num_files++;
        }
#endif

Steps to reproduce.

./wavpack -c poc.wav 

PoC: poc.zip

MSAN

==32186==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x4d2875 in realloc (/home/input0/src/WavPack/cli/wavpack+0x4d2875)
    #1 0x5107ba in main /home/input0/src/WavPack/cli/wavpack.c:813:23
    #2 0x7f42b5a4bb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

Indirect leak of 17 byte(s) in 1 object(s) allocated from:
    #0 0x4d2450 in __interceptor_malloc (/home/input0/src/WavPack/cli/wavpack+0x4d2450)
    #1 0x5107e7 in main /home/input0/src/WavPack/cli/wavpack.c:814:35
    #2 0x7f42b5a4bb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

SUMMARY: AddressSanitizer: 25 byte(s) leaked in 2 allocation(s).

[dbry]

Thanks for fuzz testing WavPack and thanks for reporting this memory leak!

I think that if you look for them you will find many such “leaks” in the WavPack command-line code, especially when terminating early in the case of an error (like the one you found). It's a complex program and can allocate a lot of memory when parsing the command line (for example see the tagging options) and to free all that memory before exiting (error or not) would be error-prone, and actually a waste of CPU time because all memory is freed anyway when the process terminates. It's a little like rearranging the deck chairs on the Titanic.

You can find this debated in several places on the web, but here's my opinion. If it's straightforward to free memory before exiting, then it's a good idea because maybe the program will be converted later into a function inside a larger program. On the other hand, if freeing that memory is not obvious and would involve otherwise unnecessary complication, then I don't think it's worth it.