[FFmpeg-devel] [PATCH 3/6] avformat/matroskadec: Fix use-after-free when demuxing ProRes

[James Almer]

On 12/6/2019 8:16 PM, Andreas Rheinhardt wrote:
> ProRes in Matroska is supposed to not contain the first atom header
> (containing a size field and the tag "icpf") and therefore the Matroska
> demuxer has to recreate it; this involves an allocation and copy, of
> course. Whether the old buffer (containing the data without the atom
> header) needs to be freed or not depends upon whether it is what was
> directly read (in which case it is owned by an AVBuffer) or whether it
> has been allocated when reversing the track's content compression (e.g.
> zlib compression) that Matroska supports.
> 
> So there are three pointers involved: The one pointing to the directly
> read data (owned by the AVBuffer), the one pointing to the currently
> valid data (which coincides with the former if no content compression
> needed to be reverted) and the one pointing to the new data with the
> first atom header. The check for whether to free the second of these is
> simply whether the first two are different.
> 
> This works mostly, but there is a complication: Some muxers don't strip
> the first atom header away and in this case, it is also not reinserted
> and no new buffer is allocated; instead, the second and the third
> pointers agree. In this case, one must never free the second buffer.
> Yet it is currently done if the track is e.g. zlib compressed.
> This commit fixes this.
> 
> This is a regression since b8e75a2a.

Nice find, and somewhat obscure :p

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> A sample can be easily created by compressing ProRes_FromHaali.mkv
> (see https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3434/) with zlib
> with mkvmerge.

Curious how mkvmerge doesn't remove the atom (if present) when the
source is another Matroska file, but it does when the source is for
example a mov.
Guess it doesn't expect it in Matroska sources taking into account the
spec, so it remuxes everything blindly. But the result is that it
doesn't even make sure the output it creates is compliant. Maybe it
should be reported as a bug?

> 
>  libavformat/matroskadec.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 080a839b6a..d1a0a07782 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -3240,11 +3240,8 @@ fail:
>  static int matroska_parse_prores(MatroskaTrack *track, uint8_t *src,
>                                   uint8_t **pdst, int *size)
>  {
> -    uint8_t *dst = src;
> -    int dstlen = *size;
> -
> -    if (AV_RB32(&src[4]) != MKBETAG('i', 'c', 'p', 'f')) {
> -        dstlen += 8;
> +    uint8_t *dst;
> +    int dstlen = *size + 8;
>  
>          dst = av_malloc(dstlen + AV_INPUT_BUFFER_PADDING_SIZE);
>          if (!dst)
> @@ -3254,7 +3251,6 @@ static int matroska_parse_prores(MatroskaTrack *track, uint8_t *src,
>          AV_WB32(dst + 4, MKBETAG('i', 'c', 'p', 'f'));
>          memcpy(dst + 8, src, dstlen - 8);
>          memset(dst + dstlen, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> -    }
>  
>      *pdst = dst;
>      *size = dstlen;
> @@ -3409,7 +3405,8 @@ static int matroska_parse_frame(MatroskaDemuxContext *matroska,
>          pkt_data = wv_data;
>      }
>  
> -    if (st->codecpar->codec_id == AV_CODEC_ID_PRORES) {
> +    if (st->codecpar->codec_id == AV_CODEC_ID_PRORES &&
> +        AV_RB32(pkt_data + 4)  != MKBETAG('i', 'c', 'p', 'f')) {
>          uint8_t *pr_data;
>          res = matroska_parse_prores(track, pkt_data, &pr_data, &pkt_size);
>          if (res < 0) {

Added a test and pushed. Thank you.