Hi,
On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer <mich...@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 <mich...@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 <mich...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
