[FFmpeg-devel] [PATCH] oggparsedaala: reject too large gpshift

[Michael Niedermayer]

On Wed, Dec 30, 2015 at 01:00:43AM +0100, Andreas Cadhalpun wrote:
> On 29.12.2015 22:27, Rostislav Pehlivanov wrote:
> > oggparsetheora has the same bit of code to read the gpshift, so it would
> > probably be a good idea to add it to this patch as well.
> 
> No, oggparsetheora only reads 5 bits for gpshift.
> The only thing from this patch that also applies there is the (theoretical)
> issue of 1<<31 not being defined for int32_t.
> 
> On 29.12.2015 22:32, Hendrik Leppkes wrote:
> > 1U << hdr->gpshift?
> 
> Sure. Updated patch attached.
> 
> Best regards,
> Andreas

>  oggparsedaala.c  |    7 ++++++-
>  oggparsetheora.c |    2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> ced39ad8585220f35fcac769d9fa0244c5a28aed  0001-oggparsedaala-reject-too-large-gpshift.patch
> From 4380123388f38eb9bbd11db34b0ac82a9ec18d5a Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> Date: Tue, 29 Dec 2015 18:32:01 +0100
> Subject: [PATCH] oggparsedaala: reject too large gpshift
> 
> Also use a unsigned constant for the shift calculation, as 1 << 31 is
> undefined for int32_t. This is also fixed oggparsetheora.
> 
> This fixes ubsan runtime error: shift exponent is too large for
> 32-bit type 'int'
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> ---
>  libavformat/oggparsedaala.c  | 7 ++++++-
>  libavformat/oggparsetheora.c | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/oggparsedaala.c b/libavformat/oggparsedaala.c
> index 24567f9..3651ca1 100644
> --- a/libavformat/oggparsedaala.c
> +++ b/libavformat/oggparsedaala.c
> @@ -123,7 +123,12 @@ static int daala_header(AVFormatContext *s, int idx)
>  
>          hdr->frame_duration = bytestream2_get_ne32(&gb);
>          hdr->gpshift = bytestream2_get_byte(&gb);
> -        hdr->gpmask  = (1 << hdr->gpshift) - 1;
> +        if (hdr->gpshift >= 32) {
> +            av_log(s, AV_LOG_ERROR, "Too large gpshift %d (>= 32).\n",
> +                   hdr->gpshift);
> +            return AVERROR_INVALIDDATA;
> +        }
> +        hdr->gpmask  = (1U << hdr->gpshift) - 1;
>  
>          hdr->format.depth  = 8 + 2*(bytestream2_get_byte(&gb)-1);
>  

> diff --git a/libavformat/oggparsetheora.c b/libavformat/oggparsetheora.c
> index 6e6a362..5f057c3 100644
> --- a/libavformat/oggparsetheora.c
> +++ b/libavformat/oggparsetheora.c
> @@ -108,7 +108,7 @@ static int theora_header(AVFormatContext *s, int idx)
>              skip_bits(&gb, 2);
>  
>          thp->gpshift = get_bits(&gb, 5);
> -        thp->gpmask  = (1 << thp->gpshift) - 1;
> +        thp->gpmask  = (1U << thp->gpshift) - 1;
>  
>          st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
>          st->codec->codec_id   = AV_CODEC_ID_THEORA;

LGTM

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160102/f2de30f9/attachment.sig>