Skip to content

fix use-after-free in WavpackUnpackSamples after a failed seek#254

Open
aizu-m wants to merge 1 commit into
dbry:masterfrom
aizu-m:unpack-seek-fail-use-after-free
Open

fix use-after-free in WavpackUnpackSamples after a failed seek#254
aizu-m wants to merge 1 commit into
dbry:masterfrom
aizu-m:unpack-seek-fail-use-after-free

Conversation

@aizu-m

@aizu-m aizu-m commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Found while fuzzing the seek-then-decode path. The project fuzzer seeks but never decodes afterwards, so this stayed out of OSS-Fuzz reach.

  1. WavpackSeekSample64 calls free_streams when the target sample is outside the current block; free_single_stream frees wps->blockbuff and sets it NULL, but wps->wphdr still describes that block and wps->wvbits still points into the freed buffer.
  2. On a failed seek (find_sample returns -1, or a later block read comes up short) the function returns FALSE and leaves that stale state behind.
  3. The next WavpackUnpackSamples decides whether to load a new block from wphdr alone, so it skips the read and decodes straight out of the freed buffer. ASan flags a heap-use-after-free in get_words_lossless.

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.

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.
@dbry

dbry commented Jun 24, 2026

Copy link
Copy Markdown
Owner

The libwavpack documentation explicitly states the following regarding WavpackSeekSample64():

After a FALSE return the file should not be accessed again
(other than to close it); this is a fatal error.

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:

WavPack/cli/wvunpack.c

Lines 1750 to 1754 in 6b7c38c

if (skip_sample_index && !WavpackSeekSample64 (wpc, skip_sample_index)) {
error_line ("can't seek to specified --skip point!");
WavpackCloseFile (wpc);
return WAVPACK_SOFT_ERROR;
}

@aizu-m

aizu-m commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

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 !wps->blockbuff branch in unpack_seek.c either mallocs and reads it, or the early-out keeps the existing buffer. So the new !wps->blockbuff term can only be true when there's no buffered block, which never happens on a good seek. It's the same guard WavpackSeekSample64 already uses internally, so it shouldn't touch the valid path.

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.

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.

2 participants