| Message ID | 20170222124214.19183-1-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: 647/clusterfuzz-testcase-5195745823031296 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/h264_ps.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > index 8090178395..f4a4a01fbe 100644 > --- a/libavcodec/h264_ps.c > +++ b/libavcodec/h264_ps.c > @@ -828,6 +828,12 @@ int ff_h264_decode_picture_parameter_set(GetBitContext > *gb, AVCodecContext *avct > pps->init_qp = get_se_golomb(gb) + 26 + > qp_bd_offset; > pps->init_qs = get_se_golomb(gb) + 26 + > qp_bd_offset; > pps->chroma_qp_index_offset[0] = get_se_golomb(gb); > + if (pps->chroma_qp_index_offset[0] < -12 || > pps->chroma_qp_index_offset[0] > 12) { > + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[0] %d is This is nonsense. Please remove. > invalid\n", pps->chroma_qp_index_offset[0]); > + ret = AVERROR_INVALIDDATA; > + goto fail; > + } > + > pps->deblocking_filter_parameters_present = get_bits1(gb); > pps->constrained_intra_pred = get_bits1(gb); > pps->redundant_pic_cnt_present = get_bits1(gb); > @@ -845,6 +851,11 @@ int ff_h264_decode_picture_parameter_set(GetBitContext > *gb, AVCodecContext *avct > pps->scaling_matrix4, > pps->scaling_matrix8); > // second_chroma_qp_index_offset > pps->chroma_qp_index_offset[1] = get_se_golomb(gb); > + if (pps->chroma_qp_index_offset[1] < -12 || > pps->chroma_qp_index_offset[1] > 12) { > + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[1] %d is > invalid\n", pps->chroma_qp_index_offset[1]); ditto. > + ret = AVERROR_INVALIDDATA; > + goto fail; > + } > } else { > pps->chroma_qp_index_offset[1] = pps->chroma_qp_index_offset[0]; > } > -- > 2.11.0 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > Having log message for every error case would make ffmpeg binary very big and slow.
On Wed, Feb 22, 2017 at 02:29:00PM +0100, Paul B Mahol wrote: > On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > Fixes: 647/clusterfuzz-testcase-5195745823031296 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/h264_ps.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > > index 8090178395..f4a4a01fbe 100644 > > --- a/libavcodec/h264_ps.c > > +++ b/libavcodec/h264_ps.c > > @@ -828,6 +828,12 @@ int ff_h264_decode_picture_parameter_set(GetBitContext > > *gb, AVCodecContext *avct > > pps->init_qp = get_se_golomb(gb) + 26 + > > qp_bd_offset; > > pps->init_qs = get_se_golomb(gb) + 26 + > > qp_bd_offset; > > pps->chroma_qp_index_offset[0] = get_se_golomb(gb); > > + if (pps->chroma_qp_index_offset[0] < -12 || > > pps->chroma_qp_index_offset[0] > 12) { > > + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[0] %d is > > This is nonsense. Please remove. Id like to keep the error messages or id like to have someone else take over h264 maintaince in ffmpeg. [...] > Having log message for every error case would make ffmpeg binary very > big and slow. I think the effect of the strings and not executed calls is negligible but iam not opposed to use a ff_elog() or av_elog() or avpriv_elog() if such macro would exist. Is anyone against adding and using such a macro ? [...]
On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Feb 22, 2017 at 02:29:00PM +0100, Paul B Mahol wrote: >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > Fixes: 647/clusterfuzz-testcase-5195745823031296 >> > >> > Found-by: continuous fuzzing process >> > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > --- >> > libavcodec/h264_ps.c | 11 +++++++++++ >> > 1 file changed, 11 insertions(+) >> > >> > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c >> > index 8090178395..f4a4a01fbe 100644 >> > --- a/libavcodec/h264_ps.c >> > +++ b/libavcodec/h264_ps.c >> > @@ -828,6 +828,12 @@ int >> > ff_h264_decode_picture_parameter_set(GetBitContext >> > *gb, AVCodecContext *avct >> > pps->init_qp = get_se_golomb(gb) + 26 >> > + >> > qp_bd_offset; >> > pps->init_qs = get_se_golomb(gb) + 26 >> > + >> > qp_bd_offset; >> > pps->chroma_qp_index_offset[0] = get_se_golomb(gb); >> > + if (pps->chroma_qp_index_offset[0] < -12 || >> > pps->chroma_qp_index_offset[0] > 12) { >> > + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[0] %d is >> >> This is nonsense. Please remove. > > Id like to keep the error messages or id like to have someone else > take over h264 maintaince in ffmpeg. Than why you are posting patches at all, if you do not want to listen to reviews. > > > [...] > >> Having log message for every error case would make ffmpeg binary very >> big and slow. > > I think the effect of the strings and not executed calls is negligible > but iam not opposed to use a ff_elog() or av_elog() or avpriv_elog() > if such macro would exist. > > Is anyone against adding and using such a macro ?
On Wed, Feb 22, 2017 at 04:41:43PM +0100, Paul B Mahol wrote: > On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, Feb 22, 2017 at 02:29:00PM +0100, Paul B Mahol wrote: > >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> > Fixes: 647/clusterfuzz-testcase-5195745823031296 > >> > > >> > Found-by: continuous fuzzing process > >> > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> > --- > >> > libavcodec/h264_ps.c | 11 +++++++++++ > >> > 1 file changed, 11 insertions(+) > >> > > >> > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > >> > index 8090178395..f4a4a01fbe 100644 > >> > --- a/libavcodec/h264_ps.c > >> > +++ b/libavcodec/h264_ps.c > >> > @@ -828,6 +828,12 @@ int > >> > ff_h264_decode_picture_parameter_set(GetBitContext > >> > *gb, AVCodecContext *avct > >> > pps->init_qp = get_se_golomb(gb) + 26 > >> > + > >> > qp_bd_offset; > >> > pps->init_qs = get_se_golomb(gb) + 26 > >> > + > >> > qp_bd_offset; > >> > pps->chroma_qp_index_offset[0] = get_se_golomb(gb); > >> > + if (pps->chroma_qp_index_offset[0] < -12 || > >> > pps->chroma_qp_index_offset[0] > 12) { > >> > + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[0] %d is > >> > >> This is nonsense. Please remove. > > > > Id like to keep the error messages or id like to have someone else > > take over h264 maintaince in ffmpeg. > > Than why you are posting patches at all, if you do not want to listen > to reviews. ive posted a patchset with ff_elog() instead do people prefer that ? [...]
On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Feb 22, 2017 at 04:41:43PM +0100, Paul B Mahol wrote: >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > On Wed, Feb 22, 2017 at 02:29:00PM +0100, Paul B Mahol wrote: >> >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> > Fixes: 647/clusterfuzz-testcase-5195745823031296 >> >> > >> >> > Found-by: continuous fuzzing process >> >> > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> >> > --- >> >> > libavcodec/h264_ps.c | 11 +++++++++++ >> >> > 1 file changed, 11 insertions(+) >> >> > >> >> > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c >> >> > index 8090178395..f4a4a01fbe 100644 >> >> > --- a/libavcodec/h264_ps.c >> >> > +++ b/libavcodec/h264_ps.c >> >> > @@ -828,6 +828,12 @@ int >> >> > ff_h264_decode_picture_parameter_set(GetBitContext >> >> > *gb, AVCodecContext *avct >> >> > pps->init_qp = get_se_golomb(gb) + >> >> > 26 >> >> > + >> >> > qp_bd_offset; >> >> > pps->init_qs = get_se_golomb(gb) + >> >> > 26 >> >> > + >> >> > qp_bd_offset; >> >> > pps->chroma_qp_index_offset[0] = get_se_golomb(gb); >> >> > + if (pps->chroma_qp_index_offset[0] < -12 || >> >> > pps->chroma_qp_index_offset[0] > 12) { >> >> > + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[0] %d >> >> > is >> >> >> >> This is nonsense. Please remove. >> > >> > Id like to keep the error messages or id like to have someone else >> > take over h264 maintaince in ffmpeg. >> >> Than why you are posting patches at all, if you do not want to listen >> to reviews. > > ive posted a patchset with ff_elog() instead > > do people prefer that ? > > [...] You could right now start adding such messages at every error path.
On Wed, Feb 22, 2017 at 07:40:24PM +0100, Paul B Mahol wrote: > On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, Feb 22, 2017 at 04:41:43PM +0100, Paul B Mahol wrote: > >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> > On Wed, Feb 22, 2017 at 02:29:00PM +0100, Paul B Mahol wrote: > >> >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> >> > Fixes: 647/clusterfuzz-testcase-5195745823031296 > >> >> > > >> >> > Found-by: continuous fuzzing process > >> >> > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> >> > --- > >> >> > libavcodec/h264_ps.c | 11 +++++++++++ > >> >> > 1 file changed, 11 insertions(+) > >> >> > > >> >> > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > >> >> > index 8090178395..f4a4a01fbe 100644 > >> >> > --- a/libavcodec/h264_ps.c > >> >> > +++ b/libavcodec/h264_ps.c > >> >> > @@ -828,6 +828,12 @@ int > >> >> > ff_h264_decode_picture_parameter_set(GetBitContext > >> >> > *gb, AVCodecContext *avct > >> >> > pps->init_qp = get_se_golomb(gb) + > >> >> > 26 > >> >> > + > >> >> > qp_bd_offset; > >> >> > pps->init_qs = get_se_golomb(gb) + > >> >> > 26 > >> >> > + > >> >> > qp_bd_offset; > >> >> > pps->chroma_qp_index_offset[0] = get_se_golomb(gb); > >> >> > + if (pps->chroma_qp_index_offset[0] < -12 || > >> >> > pps->chroma_qp_index_offset[0] > 12) { > >> >> > + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[0] %d > >> >> > is > >> >> > >> >> This is nonsense. Please remove. > >> > > >> > Id like to keep the error messages or id like to have someone else > >> > take over h264 maintaince in ffmpeg. > >> > >> Than why you are posting patches at all, if you do not want to listen > >> to reviews. > > > > ive posted a patchset with ff_elog() instead > > > > do people prefer that ? > > > > [...] > > You could right now start adding such messages at every error path. ill push my patch without the error messages, i think i failed to realize how much the error messages annoy you. [...]
On 2/23/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Feb 22, 2017 at 07:40:24PM +0100, Paul B Mahol wrote: >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > On Wed, Feb 22, 2017 at 04:41:43PM +0100, Paul B Mahol wrote: >> >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> > On Wed, Feb 22, 2017 at 02:29:00PM +0100, Paul B Mahol wrote: >> >> >> On 2/22/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> >> > Fixes: 647/clusterfuzz-testcase-5195745823031296 >> >> >> > >> >> >> > Found-by: continuous fuzzing process >> >> >> > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg >> >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> >> >> > --- >> >> >> > libavcodec/h264_ps.c | 11 +++++++++++ >> >> >> > 1 file changed, 11 insertions(+) >> >> >> > >> >> >> > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c >> >> >> > index 8090178395..f4a4a01fbe 100644 >> >> >> > --- a/libavcodec/h264_ps.c >> >> >> > +++ b/libavcodec/h264_ps.c >> >> >> > @@ -828,6 +828,12 @@ int >> >> >> > ff_h264_decode_picture_parameter_set(GetBitContext >> >> >> > *gb, AVCodecContext *avct >> >> >> > pps->init_qp = get_se_golomb(gb) >> >> >> > + >> >> >> > 26 >> >> >> > + >> >> >> > qp_bd_offset; >> >> >> > pps->init_qs = get_se_golomb(gb) >> >> >> > + >> >> >> > 26 >> >> >> > + >> >> >> > qp_bd_offset; >> >> >> > pps->chroma_qp_index_offset[0] = >> >> >> > get_se_golomb(gb); >> >> >> > + if (pps->chroma_qp_index_offset[0] < -12 || >> >> >> > pps->chroma_qp_index_offset[0] > 12) { >> >> >> > + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[0] >> >> >> > %d >> >> >> > is >> >> >> >> >> >> This is nonsense. Please remove. >> >> > >> >> > Id like to keep the error messages or id like to have someone else >> >> > take over h264 maintaince in ffmpeg. >> >> >> >> Than why you are posting patches at all, if you do not want to listen >> >> to reviews. >> > >> > ive posted a patchset with ff_elog() instead >> > >> > do people prefer that ? >> > >> > [...] >> >> You could right now start adding such messages at every error path. > > ill push my patch without the error messages, i think i failed to > realize how much the error messages annoy you. You can apply elog variant if you really need it.
diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index 8090178395..f4a4a01fbe 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -828,6 +828,12 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct pps->init_qp = get_se_golomb(gb) + 26 + qp_bd_offset; pps->init_qs = get_se_golomb(gb) + 26 + qp_bd_offset; pps->chroma_qp_index_offset[0] = get_se_golomb(gb); + if (pps->chroma_qp_index_offset[0] < -12 || pps->chroma_qp_index_offset[0] > 12) { + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[0] %d is invalid\n", pps->chroma_qp_index_offset[0]); + ret = AVERROR_INVALIDDATA; + goto fail; + } + pps->deblocking_filter_parameters_present = get_bits1(gb); pps->constrained_intra_pred = get_bits1(gb); pps->redundant_pic_cnt_present = get_bits1(gb); @@ -845,6 +851,11 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct pps->scaling_matrix4, pps->scaling_matrix8); // second_chroma_qp_index_offset pps->chroma_qp_index_offset[1] = get_se_golomb(gb); + if (pps->chroma_qp_index_offset[1] < -12 || pps->chroma_qp_index_offset[1] > 12) { + av_log(avctx, AV_LOG_ERROR, "chroma_qp_index_offset[1] %d is invalid\n", pps->chroma_qp_index_offset[1]); + ret = AVERROR_INVALIDDATA; + goto fail; + } } else { pps->chroma_qp_index_offset[1] = pps->chroma_qp_index_offset[0]; }
Fixes: 647/clusterfuzz-testcase-5195745823031296 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/h264_ps.c | 11 +++++++++++ 1 file changed, 11 insertions(+)