| Message ID | 20180124033450.4520-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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.
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.
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 [...]
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 [...]
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.
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.
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?
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 [...]
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 [...]
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); }
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(+)