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

[aizu-m]

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.

[dbry]

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]

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.

[aizu-m]

any thoughts?

[dbry]

Sorry for the delay on this; I've been occupied with another project.

The change to fuzzer.cc is definitely wrong and the regression file doesn't make sense either.

The change itself seems to be safe, but I could not convince myself that it is 100% safe.

And there's a little more of a philosophical question. If the user ignores the documentation and continues to access a file after a failed seek, what would be the best response? I think it would be better to crash right away than return incorrect audio data or some other error later on. I suppose the best thing would be to set a flag on failed seek and check it on every call into the library that could cause a problem and return an appropriate error, but that's a lot of trouble (and kind of ugly).

The docs state that access after failed seek is UB, and I'm temped to leave it at that.

[aizu-m]

Agreed on the fuzzer. The extra decode pass ran even when the seek failed, which is the exact access the docs call fatal, so the harness itself was violating the contract and would end up reporting documented behaviour as bugs. Dropped that hunk, and the regression file with it, since the file only meant anything with that pass in place. The PR is now just the blockbuff guard.

On the safety question, I traced every path that NULLs blockbuff. free_single_stream() is the only place it happens, and the only ways to re-enter WavpackUnpackSamples with wphdr still describing a live block are the failed-seek returns and the malloc-failure break inside WavpackUnpackSamples itself. That second one needs no seek at all: malloc fails after read_next_header() has filled wphdr, the function returns short, and a caller that simply calls again falls through to unpack_init(), which derefs the NULL blockbuff at open_utils.c:339. A valid decode or a successful seek always has the buffer in place, so the new term can't fire on those paths. Rebuilt and re-ran an encode/decode round trip plus a --skip decode; bit-exact both ways.

On the philosophy, no argument from me. This isn't recovery, and a flag checked on entry would be the proper semantics. The only thing I'd add is that without a sanitiser the current behaviour isn't a crash, it's a quiet read of freed heap into the caller's output buffer. But the docs do call it fatal, so if you'd rather leave it at that, no objection to closing this.