[FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy

[Anton Khirnov]

Quoting David Bryant (2020-04-06 06:11:17)
> On 4/2/20 11:32 PM, Anton Khirnov wrote:
> >
> >>> Also, I looked at the specification PDF you linked in the previous email
> >>> and I see nothing in it that explicitly forbids the channel count from
> >>> changing from one "superblock" to the next. It seems to me like you
> >>> could just concatenate wavpack files with different channel counts and
> >>> get a valid wavpack file.
> >>>
> >> You're right, I could not find that being explicitly forbidden. Nevertheless, it _is_ forbidden for any stream parameters to
> >> change (e.g. data format, sampling rate, channel count or mapping) and both libwavpack and the FFmpeg demuxer enforce this (see
> >> libavformat/wvdec.c starting at line 211), or at least gracefully fail without crashing.
> > Forbidden by whom? From what I can see:
> > - sample format: the wv demuxer does not set it at all, the decoder sets
> >   it in wavpack_decode_frame() from the block header; also, Michael sent
> >   a patch recently to deal with a wavpack decoder crash related to
> >   format changes
> > - sample rate: set by wv demuxer and then checked that it does not
> >   change; but the caller is not necessarily using the wv demuxer and the
> >   decoder will happily change it the same way it does with channel count
> >
> >> The reason it's never mentioned is probably just because it never occurred to me. None of the source file formats for WavPack
> >> (e.g., wav, caf, dsdiff) allow format changes and neither do any of the other lossless compressors I am aware of.
> > Correct, I cannot see any other lossless codecs that would allow this.
> > But then again all these codecs have global headers. So it's not that
> > they artificially forbid format changes, but the bistream format is not
> > able to represent them.
> 
> I disagree with that. It would be trivial to add a new STREAMINFO block to a FLAC file and change the format after that. This is
> not specifically prohibited in the FLAC specs, but it's sort of implied when it says that the initial STREAMINFO block has
> "information about the whole stream".

>From a quick glance at the spec, metadata blocks are only allowed at the
beginning and are followed by audio frames. Once you see a single audio
frame, there should be no further metadata blocks.

That said, the frame header does contain stream parameters and those can
conceivably change and after looking at our decoder, it does support
those changes. It was added explicitly in
commit 90fcac0e95b7d266c148a86506f301a2072d9de3
Author: Justin Ruggles <justin.ruggles at gmail.com>
Date:   Sun Oct 21 17:02:28 2012 -0400

    flacdec: allow mid-stream channel layout change
    
    Although the libFLAC decoder cannot handle such a change, it is allowed by the
    spec and could potentially occur with live streams.

> 
> >
> >> and extensive fuzzing has ensured that pathological files like you
> >> suggest are unlikely to be the source of exploits.
> > See the crash above. I think fuzzing is very overhyped these days. It's
> > a useful tool for sure, but one should not rely purely on fuzzing too
> > much. It may not cover important parts of the input space because of the
> > way it is set up. Or there could be exploitable codepaths that require
> > too specific inputs for a random process to find, but a determined
> > attacker could construct them from reading the code.
> >
> Of course fuzzing cannot find everything, but my experience has been that fuzzing finds things that I would _never_ find by just
> looking at the code.
> 
> And I was certainly not suggesting that one should just write sloppy code knowing that all the bugs will be found with fuzzing.

Yeah, I certainly agree that fuzzing is a very useful tool. My point is
just that it shouldn't be your _only_ tool. There can be plenty of bugs
fuzzing won't find.

-- 
Anton Khirnov