fix use-after-free in WavpackUnpackSamples after a failed seek#254
fix use-after-free in WavpackUnpackSamples after a failed seek#254aizu-m wants to merge 1 commit into
Conversation
A failed WavpackSeekSample64() frees the stream's raw block buffer but leaves wphdr describing a valid block, so the next decode read from freed memory. Treat a NULL blockbuff as no current block so a fresh block is read instead.
|
The libwavpack documentation explicitly states the following regarding WavpackSeekSample64(): The reason I did this is I thought it would greatly complicate the seeking functionality to be able to safely recover from all possible seeking failures, and the only reason that seek should fail is a corrupt file. Are you suggesting that your tiny fix actually makes the seeking and decoding fully functional after a seeking failure? Or does it just fix that one exception? That code is complicated enough that it's not obvious, and testing all conditions around failed seeks would also be complicated. I certainly would not like to see some issue introduced to valid seeking by this change! You also mention that the wvunpack CLI program can encounter this, but the wvunpack program specifically tests for a failed seek and terminates: Lines 1750 to 1754 in 6b7c38c |
|
You're right on the CLI, I overstated that. wvunpack checks the WavpackSeekSample64 return and bails with WAVPACK_SOFT_ERROR, so it never decodes after a failed seek. The path that actually reaches this is a direct library caller doing seek-then-unpack, which is the case the docs warn against. On scope: it only fixes the one exception, not recovery. After a failed seek the stream state is still junk. All the change does is stop WavpackUnpackSamples decoding out of the freed blockbuff. When blockbuff is NULL it now reads a fresh block instead, so the access becomes defined rather than a heap-use-after-free. It doesn't make post-failure seeking work, and I wouldn't claim it does. On valid seeking: a successful seek always leaves blockbuff non-NULL. The So in practice it just turns the UB into a clean read for a caller that ignores the don't-access-after-FALSE contract. Given the docs call that a fatal error anyway, I'm fine if you'd rather leave it as documented. |
Found while fuzzing the seek-then-decode path. The project fuzzer seeks but never decodes afterwards, so this stayed out of OSS-Fuzz reach.
Reachable from the public API (seek then unpack) and from the CLI as wvunpack --skip= file.wv on a truncated or corrupt file where the seek fails.
Added !wps->blockbuff to that decision so a missing buffer forces a fresh read, which is the same !wps->blockbuff check WavpackSeekSample64 already makes internally. fuzzer.cc now decodes after the seek so the regressors exercise this path, and the regression file aborts the address-sanitiser build before the change and decodes clean after.