Created attachment 340139 [details] [review]
Support for FLAC in ISOBMFF

An example DASH stream can be found at http://rdmedia.bbc.co.uk/dash/ondemand/testcard/radio/1/client_manifest-flac.mpd

The output of qtdemux can still be fed to flacparse, or alternatively can passed to flacdec or avdec_flac directly.

Review of attachment 340139 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +10940,3 @@
+                 * METADATA_BLOCK_STREAMINFO block */
+                stream->rate =
+                    (QT_UINT32 (metadata_blocks + 14) >> 12) & 0xFFFFF;

There are no checks that this much memory is available. Also in the other array accesses above.

Is the sample rate guaranteed to be given in the STREAMINFO block, or can it be 0 and only be provided via the container?


You might also want to put channel, samplerate and other information (as flacparse does) into the caps

Thanks for such a quick initial review.

> There are no checks that this much memory is available. Also in the other array accesses above.

The available data length is extracted from the dfla box header and checked to be at least a minimum valid fifty bytes. After that, all array accesses are either programmatically limited or hard-coded to values less than the reported length. Looking at several other box parsers above (eg alac, wma), they operate in an identical way. Should I be approaching this differently?

> You might also want to put channel, samplerate and other information (as flacparse does) into the caps

These values are extracted by the stsd parser and added to the caps automatically in gst_qtdemux_configure_stream so they do end up on the final caps. The isoflac spec requires that the STREAMINFO values match the stsd values so there didn't seem any point doing extra work here. I am of course happy to do this explicitly here if you wish, or have I missed the point?

> Is the sample rate guaranteed to be given in the STREAMINFO block, or can it be 0 and only be provided via the container?

0 is called out explicitly invalid in the FLAC format specification - it must be supplied and correct in the STREAMINFO.

(In reply to David Evans from comment #4)
> Thanks for such a quick initial review.
> 
> > There are no checks that this much memory is available. Also in the other array accesses above.
> 
> The available data length is extracted from the dfla box header and checked
> to be at least a minimum valid fifty bytes. After that, all array accesses
> are either programmatically limited or hard-coded to values less than the
> reported length. Looking at several other box parsers above (eg alac, wma),
> they operate in an identical way. Should I be approaching this differently?

Seems ok then, the dfla box length is already validated when parsing the box earlier. Seems I misread :)

> > You might also want to put channel, samplerate and other information (as flacparse does) into the caps
> 
> These values are extracted by the stsd parser and added to the caps
> automatically in gst_qtdemux_configure_stream so they do end up on the final
> caps. The isoflac spec requires that the STREAMINFO values match the stsd
> values so there didn't seem any point doing extra work here. I am of course
> happy to do this explicitly here if you wish, or have I missed the point?
> 
> > Is the sample rate guaranteed to be given in the STREAMINFO block, or can it be 0 and only be provided via the container?
> 
> 0 is called out explicitly invalid in the FLAC format specification - it
> must be supplied and correct in the STREAMINFO.

Seems good then

commit 2ad30254c330d049e60f3457d305571af3cc4d6d
Author: David Evans <bbcrddave@gmail.com>
Date:   Thu Nov 17 13:59:48 2016 +0000

    qtdemux: Add support for FLAC encapsulated in ISOBMFF
    
    As defined by
    https://git.xiph.org/?p=flac.git;a=blob_plain;f=doc/isoflac.txt
    
    https://bugzilla.gnome.org/show_bug.cgi?id=773712

Created attachment 340433 [details] [review]
Ensure all dfLa array accesses are constrained to box length

> Also in the other array accesses above.

Having thought about this much more over the weekend, it turns out you were right Sebastian. It turned out that the last iteration through the while loop could indeed exceed the box length if the data was corrupt/incorrect/malicious. So, many apologies for that.

This patch ensures that as the metadata blocks are accessed, all reads are constrained to within the box length which was validated earlier.

Note that in order for downstream elements to work correctly, the last-metadata-block flag must be set. In the case that the data is corrupt, this will not have happened so it's better not to set anything in the streamheader.

commit 45843ab9a2d883607f2f9e54c1c3c143e0c7ec2f
Author: David Evans <bbcrddave@gmail.com>
Date:   Mon Nov 21 15:25:23 2016 +0000

    qtdemux: Be sure not to read off end of FLAC dfLa box
    
    https://bugzilla.gnome.org/show_bug.cgi?id=773712