[FFmpeg-devel] [PATCH] Chromium MKV patches

[Aurelien Jacobs]

On Wed, May 05, 2010 at 05:18:20AM -0400, David Conrad wrote:
> Hi,
> 
> Here are the useful patches from chromium for the mkv demuxer. 1 doesn't fix any actual issues that I'm aware of, but might make it easier to spot any if they exist.
> 
> Another change made was ensuring sample_rate was nonzero, but AFAIK a zero sample_rate meant unknown and applications should handle that instead of the demuxer inventing a rate?
> 

> From ccd07e7b93df77882cd5f69c00c32af728a6545e Mon Sep 17 00:00:00 2001
> From: David Conrad <lessen42 at gmail.com>
> Date: Sat, 1 May 2010 14:05:14 -0400
> Subject: [PATCH 1/4] matroskadec: Use av_freep in ebml_read_ascii
> 
> Based on a Chromium patch
> ---
>  libavformat/matroskadec.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 4e6f375..46d039a 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -615,13 +615,13 @@ static int ebml_read_float(ByteIOContext *pb, int size, double *num)
>   */
>  static int ebml_read_ascii(ByteIOContext *pb, int size, char **str)
>  {
> -    av_free(*str);
> +    av_freep(str);
>      /* EBML strings are usually not 0-terminated, so we allocate one
>       * byte more, read the string and NULL-terminate it ourselves. */
>      if (!(*str = av_malloc(size + 1)))
>          return AVERROR(ENOMEM);

This hunk is rejected.
There is no point in zeroing *str when it's overwritten just on the next
line.

>      if (get_buffer(pb, (uint8_t *) *str, size) != size) {
> -        av_free(*str);
> +        av_freep(str);
>          return AVERROR(EIO);
>      }
>      (*str)[size] = '\0';

Might make debugging easier if something wrong happen, so OK.

> From fd0192fc730621b16f851d1e47bc854a63b1d4a9 Mon Sep 17 00:00:00 2001
> From: David Conrad <lessen42 at gmail.com>
> Date: Sat, 1 May 2010 14:13:01 -0400
> Subject: [PATCH 2/4] matroskadec: Ensure time_scale is nonzero, fixes divide-by-zero if the file
>  has 0 written
> 
> Based on a Chromium patch
> ---
>  libavformat/matroskadec.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 46d039a..2b4d513 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1154,6 +1154,8 @@ static int matroska_read_header(AVFormatContext *s, AVFormatParameters *ap)
>          return -1;
>      matroska_execute_seekhead(matroska);
>  
> +    if (!matroska->time_scale)
> +        matroska->time_scale = 1000000;
>      if (matroska->duration)
>          matroska->ctx->duration = matroska->duration * matroska->time_scale
>                                    * 1000 / AV_TIME_BASE;

A file containing time_scale=0 is broken.
But if they wrote this patch, I guess such a file existe in the wild, so
patch OK.
Having a sample would of course be useful...

> From 3c8b0a5a4f3843f0f6f9d31ac8e8a486ced27d62 Mon Sep 17 00:00:00 2001
> From: David Conrad <lessen42 at gmail.com>
> Date: Sat, 1 May 2010 14:17:57 -0400
> Subject: [PATCH 3/4] matroskadec: Fix buffer overread in matroska_ebmlnum_uint
> 
> Based on a Chromium patch
> ---
>  libavformat/matroskadec.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 2b4d513..8e70bbc 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -679,7 +679,7 @@ static int matroska_ebmlnum_uint(MatroskaDemuxContext *matroska,
>  {
>      ByteIOContext pb;
>      init_put_byte(&pb, data, size, 0, NULL, NULL, NULL, NULL);
> -    return ebml_read_num(matroska, &pb, 8, num);
> +    return ebml_read_num(matroska, &pb, FFMIN(size, 8), num);
>  }
>  
>  /*

Should be OK.

> From 35c4685b5b08f961522ead0a7b4b534dce552ff6 Mon Sep 17 00:00:00 2001
> From: David Conrad <lessen42 at gmail.com>
> Date: Sat, 1 May 2010 14:42:12 -0400
> Subject: [PATCH 4/4] matroskadec: Free ebml binary buffer on error
> 
> Based on a Chromium patch
> ---
>  libavformat/matroskadec.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 8e70bbc..e449838 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -641,8 +641,10 @@ static int ebml_read_binary(ByteIOContext *pb, int length, EbmlBin *bin)
>  
>      bin->size = length;
>      bin->pos  = url_ftell(pb);
> -    if (get_buffer(pb, bin->data, length) != length)
> +    if (get_buffer(pb, bin->data, length) != length) {
> +        av_freep(&bin->data);
>          return AVERROR(EIO);
> +    }
>  
>      return 0;
>  }

OK.

Aurel