[FFmpeg-devel,1/2] avcodec/hevc_ps: Check log2_sao_offset_scale_*

Submitted by Michael Niedermayer on Jan. 24, 2018, 3:34 a.m.

Details

Message ID 20180124033450.4520-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Jan. 24, 2018, 3:34 a.m.
Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/hevc_ps.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

wm4 Jan. 24, 2018, 3:42 a.m.
On Wed, 24 Jan 2018 04:34:49 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hevc_ps.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index 4787312cfa..746c96b17e 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
>      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
>      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
>  
> +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
> +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
> +    ) {
> +        av_log(avctx, AV_LOG_ERROR,
> +                "log2 sao offset scales (%d %d) are invalid\n",
> +               pps->log2_sao_offset_scale_luma,
> +               pps->log2_sao_offset_scale_chroma
> +              );
> +        return AVERROR_INVALIDDATA;
> +    }
> +
>      return(0);
>  }
>  

Unnecessary logging.
James Almer Jan. 24, 2018, 3:47 a.m.
On 1/24/2018 12:34 AM, Michael Niedermayer wrote:
> Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hevc_ps.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index 4787312cfa..746c96b17e 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
>      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
>      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
>  
> +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
> +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
> +    ) {
> +        av_log(avctx, AV_LOG_ERROR,
> +                "log2 sao offset scales (%d %d) are invalid\n",
> +               pps->log2_sao_offset_scale_luma,
> +               pps->log2_sao_offset_scale_chroma
> +              );
> +        return AVERROR_INVALIDDATA;

Wouldn't it be better to just port the h264 and hevc decoder to use the
cbs API at this point? It correctly does a range check for every
sps/vps/pps/slice value already.

Otherwise you'll be adding a lot of range checks as oss-fuzz finds an
ubsan testcase for them.
Michael Niedermayer Jan. 25, 2018, 2:03 a.m.
On Wed, Jan 24, 2018 at 12:47:18AM -0300, James Almer wrote:
> On 1/24/2018 12:34 AM, Michael Niedermayer wrote:
> > Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> > Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hevc_ps.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> > index 4787312cfa..746c96b17e 100644
> > --- a/libavcodec/hevc_ps.c
> > +++ b/libavcodec/hevc_ps.c
> > @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
> >      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
> >      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
> >  
> > +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
> > +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
> > +    ) {
> > +        av_log(avctx, AV_LOG_ERROR,
> > +                "log2 sao offset scales (%d %d) are invalid\n",
> > +               pps->log2_sao_offset_scale_luma,
> > +               pps->log2_sao_offset_scale_chroma
> > +              );
> > +        return AVERROR_INVALIDDATA;
> 
> Wouldn't it be better to just port the h264 and hevc decoder to use the
> cbs API at this point? It correctly does a range check for every
> sps/vps/pps/slice value already.
> 
> Otherwise you'll be adding a lot of range checks as oss-fuzz finds an
> ubsan testcase for them.

cbs is not available in the releases
we need to fix issues in the releases

so i dont think cbs can help here

[...]
Michael Niedermayer Jan. 25, 2018, 2:26 a.m.
On Wed, Jan 24, 2018 at 04:42:38AM +0100, wm4 wrote:
> On Wed, 24 Jan 2018 04:34:49 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> > Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hevc_ps.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> > index 4787312cfa..746c96b17e 100644
> > --- a/libavcodec/hevc_ps.c
> > +++ b/libavcodec/hevc_ps.c
> > @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
> >      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
> >      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
> >  
> > +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
> > +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
> > +    ) {
> > +        av_log(avctx, AV_LOG_ERROR,
> > +                "log2 sao offset scales (%d %d) are invalid\n",
> > +               pps->log2_sao_offset_scale_luma,
> > +               pps->log2_sao_offset_scale_chroma
> > +              );
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> >      return(0);
> >  }
> >  
> 
> Unnecessary logging.

i would prefer to keep the logging. But if people want it removed ill remove it.
Of course without error logging i will not be available to maintain or help
maintain hevc in the future. 

thx


[...]
wm4 Jan. 25, 2018, 2:39 a.m.
On Thu, 25 Jan 2018 03:26:51 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Wed, Jan 24, 2018 at 04:42:38AM +0100, wm4 wrote:
> > On Wed, 24 Jan 2018 04:34:49 +0100
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >   
> > > Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> > > Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/hevc_ps.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> > > index 4787312cfa..746c96b17e 100644
> > > --- a/libavcodec/hevc_ps.c
> > > +++ b/libavcodec/hevc_ps.c
> > > @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
> > >      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
> > >      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
> > >  
> > > +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
> > > +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
> > > +    ) {
> > > +        av_log(avctx, AV_LOG_ERROR,
> > > +                "log2 sao offset scales (%d %d) are invalid\n",
> > > +               pps->log2_sao_offset_scale_luma,
> > > +               pps->log2_sao_offset_scale_chroma
> > > +              );
> > > +        return AVERROR_INVALIDDATA;
> > > +    }
> > > +
> > >      return(0);
> > >  }
> > >    
> > 
> > Unnecessary logging.  
> 
> i would prefer to keep the logging. But if people want it removed ill remove it.
> Of course without error logging i will not be available to maintain or help
> maintain hevc in the future. 

Farewell, then.
James Almer Jan. 25, 2018, 2:42 a.m.
On 1/24/2018 11:03 PM, Michael Niedermayer wrote:
> On Wed, Jan 24, 2018 at 12:47:18AM -0300, James Almer wrote:
>> On 1/24/2018 12:34 AM, Michael Niedermayer wrote:
>>> Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
>>> Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/hevc_ps.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>>> index 4787312cfa..746c96b17e 100644
>>> --- a/libavcodec/hevc_ps.c
>>> +++ b/libavcodec/hevc_ps.c
>>> @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
>>>      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
>>>      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
>>>  
>>> +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
>>> +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
>>> +    ) {
>>> +        av_log(avctx, AV_LOG_ERROR,
>>> +                "log2 sao offset scales (%d %d) are invalid\n",
>>> +               pps->log2_sao_offset_scale_luma,
>>> +               pps->log2_sao_offset_scale_chroma
>>> +              );
>>> +        return AVERROR_INVALIDDATA;
>>
>> Wouldn't it be better to just port the h264 and hevc decoder to use the
>> cbs API at this point? It correctly does a range check for every
>> sps/vps/pps/slice value already.
>>
>> Otherwise you'll be adding a lot of range checks as oss-fuzz finds an
>> ubsan testcase for them.
> 
> cbs is not available in the releases
> we need to fix issues in the releases
> 
> so i dont think cbs can help here

For release branches yes, no way around it, patches like this are
needed. But for future releases it will prevent this kind of fix to be
added as fuzzers find issues. Eventually, every supported release will
be one using cbs where range checks are already implemented, so the
quickest it's done the better.
wm4 Jan. 25, 2018, 2:48 a.m.
On Wed, 24 Jan 2018 23:42:44 -0300
James Almer <jamrial@gmail.com> wrote:

> On 1/24/2018 11:03 PM, Michael Niedermayer wrote:
> > On Wed, Jan 24, 2018 at 12:47:18AM -0300, James Almer wrote:  
> >> On 1/24/2018 12:34 AM, Michael Niedermayer wrote:  
> >>> Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> >>> Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/hevc_ps.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>> index 4787312cfa..746c96b17e 100644
> >>> --- a/libavcodec/hevc_ps.c
> >>> +++ b/libavcodec/hevc_ps.c
> >>> @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
> >>>      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
> >>>      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
> >>>  
> >>> +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
> >>> +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
> >>> +    ) {
> >>> +        av_log(avctx, AV_LOG_ERROR,
> >>> +                "log2 sao offset scales (%d %d) are invalid\n",
> >>> +               pps->log2_sao_offset_scale_luma,
> >>> +               pps->log2_sao_offset_scale_chroma
> >>> +              );
> >>> +        return AVERROR_INVALIDDATA;  
> >>
> >> Wouldn't it be better to just port the h264 and hevc decoder to use the
> >> cbs API at this point? It correctly does a range check for every
> >> sps/vps/pps/slice value already.
> >>
> >> Otherwise you'll be adding a lot of range checks as oss-fuzz finds an
> >> ubsan testcase for them.  
> > 
> > cbs is not available in the releases
> > we need to fix issues in the releases
> > 
> > so i dont think cbs can help here  
> 
> For release branches yes, no way around it, patches like this are
> needed. But for future releases it will prevent this kind of fix to be
> added as fuzzers find issues. Eventually, every supported release will
> be one using cbs where range checks are already implemented, so the
> quickest it's done the better.

Is there even any indication that this is an important fix that
requires it to be backported?
Michael Niedermayer Jan. 25, 2018, 8:05 p.m.
On Wed, Jan 24, 2018 at 11:42:44PM -0300, James Almer wrote:
> On 1/24/2018 11:03 PM, Michael Niedermayer wrote:
> > On Wed, Jan 24, 2018 at 12:47:18AM -0300, James Almer wrote:
> >> On 1/24/2018 12:34 AM, Michael Niedermayer wrote:
> >>> Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> >>> Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/hevc_ps.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>> index 4787312cfa..746c96b17e 100644
> >>> --- a/libavcodec/hevc_ps.c
> >>> +++ b/libavcodec/hevc_ps.c
> >>> @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
> >>>      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
> >>>      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
> >>>  
> >>> +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
> >>> +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
> >>> +    ) {
> >>> +        av_log(avctx, AV_LOG_ERROR,
> >>> +                "log2 sao offset scales (%d %d) are invalid\n",
> >>> +               pps->log2_sao_offset_scale_luma,
> >>> +               pps->log2_sao_offset_scale_chroma
> >>> +              );
> >>> +        return AVERROR_INVALIDDATA;
> >>
> >> Wouldn't it be better to just port the h264 and hevc decoder to use the
> >> cbs API at this point? It correctly does a range check for every
> >> sps/vps/pps/slice value already.
> >>
> >> Otherwise you'll be adding a lot of range checks as oss-fuzz finds an
> >> ubsan testcase for them.
> > 
> > cbs is not available in the releases
> > we need to fix issues in the releases
> > 
> > so i dont think cbs can help here
> 
> For release branches yes, no way around it, patches like this are
> needed.

Yes and fixes to releases are bascially always backports from master
and thats what this patchset does, fix it in master in a form that can
be hopefully backported with minimal issues.


> But for future releases it will prevent this kind of fix to be
> added as fuzzers find issues. Eventually, every supported release will
> be one using cbs where range checks are already implemented, so the
> quickest it's done the better.

when all release use cbs then we will still find bugs and will have to
fix them. Both in cbs and outside.
In case of hevc that should be fewer bugs as the hevc bit stream reading
seems to have been writen more sloppy than in other decoders.
Other decoders quite possibly will have more bugs in their cbs code than
before as cbs is alot of not very much tested code, while the decoders
have been tested and fuzzed quite extensivly


[...]
Michael Niedermayer Jan. 30, 2018, 8:26 p.m.
On Wed, Jan 24, 2018 at 04:34:49AM +0100, Michael Niedermayer wrote:
> Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hevc_ps.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

applied without the error message


[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
index 4787312cfa..746c96b17e 100644
--- a/libavcodec/hevc_ps.c
+++ b/libavcodec/hevc_ps.c
@@ -1324,6 +1324,17 @@  static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
     pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
     pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
 
+    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
+        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
+    ) {
+        av_log(avctx, AV_LOG_ERROR,
+                "log2 sao offset scales (%d %d) are invalid\n",
+               pps->log2_sao_offset_scale_luma,
+               pps->log2_sao_offset_scale_chroma
+              );
+        return AVERROR_INVALIDDATA;
+    }
+
     return(0);
 }