flac icon indicating copy to clipboard operation
flac copied to clipboard

Fix out-of-memory handling

Open ktmf01 opened this issue 3 years ago • 1 comments

This should fix #215 and close #214 as redundant. It also fixes some potential problems not mentioned in #215.

The main problem is described in the second commit

Parts of the code use realloc like

x = safe_realloc(x, somesize);

when this is the case, the safe_realloc variant used must free the
old memory block in case it fails, otherwise it will leak. However,
there are also instances in the code where handling is different:

if (0 == (x = safe_realloc(y, somesize)))
    return false

in this case, y should not be freed, as y is not set to NULL we
could encounter double frees. Here the safe_realloc_nofree
functions are used.

ktmf01 avatar Aug 03 '22 18:08 ktmf01

@ltx2018, @petterreinholdtsen as you both commented on #214 and #215 and @lvqcl as you contributed a few patches for out-of-memory handling, could you spare some time to take a look at this solution to that problem? I can't find a way to simulate out-of-memory situations in fuzzing, so it would be great to have someone else look at this too.

ktmf01 avatar Aug 03 '22 20:08 ktmf01

I've found a way to verify this patch through fuzzing. I've added two global variables (declared and defined only #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION) which the fuzzer uses, a counter and a threshold. As soon as the threshold is hit, the next malloc/calloc/realloc fails. If FLAC__STREAM_ENCODER_MEMORY_ALLOCATION_ERROR or FLAC__STREAM_ENCODER_FRAMING_ERROR is returned to the fuzzer without leaking or double-freeing any memory, the fuzzer won't complain.

This added to the fuzzers turns up a double-free (first free in bitwriter_grow) without this patch, and this patch fixes it.

I'll leave this PR open for review. I plan to integrate it in a larger PR with the fuzzing additions next week

ktmf01 avatar Aug 12 '22 10:08 ktmf01

Closing this as #419 (which included these patches) has been merged

ktmf01 avatar Aug 20 '22 14:08 ktmf01