[FFmpeg-devel] [PATCH 2/2] avcodec/h264: Check weight values to be within the specs limits.

Ronald S. Bultje rsbultje at gmail.com
Wed Mar 22 15:06:09 EET 2017


Hi,

On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> @@ -59,6 +59,9 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const
> SPS *sps,
>              if (luma_weight_flag) {
>                  pwt->luma_weight[i][list][0] = get_se_golomb(gb);
>                  pwt->luma_weight[i][list][1] = get_se_golomb(gb);
> +                if (   (int8_t)pwt->luma_weight[i][list][0] !=
> pwt->luma_weight[i][list][0]
> +                    || (int8_t)pwt->luma_weight[i][list][1] !=
> pwt->luma_weight[i][list][1])
> +                    goto error;
>

Can we please put || on the line before? h264* and a very limited subset of
other files in our codebase have always been an exception to the large
majority of the codebase and it's about time we fix that [1].

It's interesting (I mean that in a positive way) how you use casting to
check for the range. It's a little obscure, but it's OK.

+error:
> +    avpriv_request_sample(logctx, "Out of range weight\n");
> +    return AVERROR_INVALIDDATA;


Same comment as previously in other, but related, threads: unless there is
real, demonstrable evidence that this happens in real-world files, this is
fuzz/corruption only and shouldn't be accompanied by an explicit log
message. Just return AVERROR_INVALIDDATA directly and remove the goto/error
message.

(Secondary comment here, which is negated by the first, but just in case
other people want to argue over this some more: using a generic label
"error" and accompanying it by a specific error that can only possibly
apply to a small subset of potential errors returned from this function is
a Bad Design. Either the label should be specific (weight_range_error:), or
the log message should be generic ("Error parsing pred weight table").
However, you can obviously ignore this since I don't believe the string
should be there at all, so in that case this comment doesn't apply.)

Ronald

[1] a grep for multi-line if statements placing ||/&& on top line in
libavcodec/:
yop, xxan, xwdenc, xan, wmv2, wmavoice, wmaprodec, wmalosslessdec, wma,
wavpackenc, wavpack, vp9, vp8, vp6, vp56, vp5, vp3dsp, vp3, vorbisdec,
vmdvideo, videotoolboxenc [ I stopped here]
a grep for multi-line if statements placing ||/&& on bottom line in
libavcodec/ in that same alphabetic range:
only one single line (inconsistency) in vp6
Therefore, || on previous line is more prevalent than on next line.

On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> Fixes: integer overflows
> Fixes: 911/clusterfuzz-testcase-5415105606975488
>
> Found-by: continuous fuzzing process https://github.com/google/oss-
> fuzz/tree/master/targets/ffmpeg
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libavcodec/h264_parse.c | 9 +++++++++
>  libavcodec/h264_slice.c | 7 +++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
> index 0c873196dc..2b7aad087a 100644
> --- a/libavcodec/h264_parse.c
> +++ b/libavcodec/h264_parse.c
> @@ -59,6 +59,9 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const
> SPS *sps,
>              if (luma_weight_flag) {
>                  pwt->luma_weight[i][list][0] = get_se_golomb(gb);
>                  pwt->luma_weight[i][list][1] = get_se_golomb(gb);
> +                if (   (int8_t)pwt->luma_weight[i][list][0] !=
> pwt->luma_weight[i][list][0]
> +                    || (int8_t)pwt->luma_weight[i][list][1] !=
> pwt->luma_weight[i][list][1])
> +                    goto error;
>                  if (pwt->luma_weight[i][list][0] != luma_def ||
>                      pwt->luma_weight[i][list][1] != 0) {
>                      pwt->use_weight             = 1;
> @@ -76,6 +79,9 @@ int ff_h264_pred_weight_table(GetBitContext *gb, const
> SPS *sps,
>                      for (j = 0; j < 2; j++) {
>                          pwt->chroma_weight[i][list][j][0] =
> get_se_golomb(gb);
>                          pwt->chroma_weight[i][list][j][1] =
> get_se_golomb(gb);
> +                        if (   (int8_t)pwt->chroma_weight[i][list][j][0]
> != pwt->chroma_weight[i][list][j][0]
> +                            || (int8_t)pwt->chroma_weight[i][list][j][1]
> != pwt->chroma_weight[i][list][j][1])
> +                            goto error;
>                          if (pwt->chroma_weight[i][list][j][0] !=
> chroma_def ||
>                              pwt->chroma_weight[i][list][j][1] != 0) {
>                              pwt->use_weight_chroma        = 1;
> @@ -104,6 +110,9 @@ int ff_h264_pred_weight_table(GetBitContext *gb,
> const SPS *sps,
>      }
>      pwt->use_weight = pwt->use_weight || pwt->use_weight_chroma;
>      return 0;
> +error:
> +    avpriv_request_sample(logctx, "Out of range weight\n");
> +    return AVERROR_INVALIDDATA;
>  }
>
>  /**
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index a703853872..c86ad1c3a6 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1781,9 +1781,12 @@ static int h264_slice_header_parse(const
> H264Context *h, H264SliceContext *sl,
>      }
>      if ((pps->weighted_pred && sl->slice_type_nos == AV_PICTURE_TYPE_P) ||
>          (pps->weighted_bipred_idc == 1 &&
> -         sl->slice_type_nos == AV_PICTURE_TYPE_B))
> -        ff_h264_pred_weight_table(&sl->gb, sps, sl->ref_count,
> +         sl->slice_type_nos == AV_PICTURE_TYPE_B)) {
> +        ret = ff_h264_pred_weight_table(&sl->gb, sps, sl->ref_count,
>                                    sl->slice_type_nos, &sl->pwt, h->avctx);
> +        if (ret < 0)
> +            return ret;
> +    }
>
>      sl->explicit_ref_marking = 0;
>      if (nal->ref_idc) {
> --
> 2.11.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list