| Message ID | 20170322015956.20074-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
Hi, On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer <michael@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@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@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 >
On Wed, Mar 22, 2017 at 09:06:09AM -0400, Ronald S. Bultje wrote: > Hi, > > On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer <michael@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]. i find putting the operators in the first column more readable but if its preferred in the last, iam happy to change it. > > 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. yes, it caused me to pause to but it was the simplest way i saw to do the check > > +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. If there is "real, demonstrable evidence that this happens in real-world files" then we would likely have a sample and not ask for one with avpriv_request_sample() I think its very plausible that a encoder would use a weight that is outside the range. Printing something does make sense. off topic, but i saw some spurious errors in h264s output that were new, maybe i can reduce these. [...]
On 2017-03-22 09:06 -0400, Ronald S. Bultje wrote: > On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer <michael@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]. No strong opinion on this one, but putting operators in front on the next line makes them stay aligned in case a name changes later on. I think it is a little bit easier to read too. I did not check the counts, but I guess putting the logical operators at the end is used far more common in our code base and in no way do I suggest we should change all of those occurrences to this style. [...] Alexander
On Wed, Mar 22, 2017 at 07:09:23PM +0100, Michael Niedermayer wrote: > On Wed, Mar 22, 2017 at 09:06:09AM -0400, Ronald S. Bultje wrote: > > Hi, > > > > On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer <michael@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]. > > i find putting the operators in the first column more readable > but if its preferred in the last, iam happy to change it. > > > > > > 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. > > yes, it caused me to pause to but it was the simplest way i saw to do > the check > > > > > > +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. > > If there is "real, demonstrable evidence that this happens in real-world > files" then we would likely have a sample and not ask for one with > avpriv_request_sample() > > I think its very plausible that a encoder would use a weight that is > outside the range. Printing something does make sense. i will apply this with the label chaned to out_range_weight: unless there are objections [...]
Hi, On Thu, Apr 6, 2017 at 8:10 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Mar 22, 2017 at 07:09:23PM +0100, Michael Niedermayer wrote: > > On Wed, Mar 22, 2017 at 09:06:09AM -0400, Ronald S. Bultje wrote: > > > Hi, > > > > > > On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer > <michael@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]. > > > > i find putting the operators in the first column more readable > > but if its preferred in the last, iam happy to change it. > > > > > > > > > > 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. > > > > yes, it caused me to pause to but it was the simplest way i saw to do > > the check > > > > > > > > > > +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. > > > > If there is "real, demonstrable evidence that this happens in real-world > > files" then we would likely have a sample and not ask for one with > > avpriv_request_sample() > > > > I think its very plausible that a encoder would use a weight that is > > outside the range. Printing something does make sense. > > i will apply this with the label chaned to out_range_weight: > unless there are objections OK. Ronald
On Thu, Apr 06, 2017 at 10:32:15PM -0400, Ronald S. Bultje wrote: > Hi, > > On Thu, Apr 6, 2017 at 8:10 PM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Wed, Mar 22, 2017 at 07:09:23PM +0100, Michael Niedermayer wrote: > > > On Wed, Mar 22, 2017 at 09:06:09AM -0400, Ronald S. Bultje wrote: > > > > Hi, > > > > > > > > On Tue, Mar 21, 2017 at 9:59 PM, Michael Niedermayer > > <michael@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]. > > > > > > i find putting the operators in the first column more readable > > > but if its preferred in the last, iam happy to change it. > > > > > > > > > > > > > > 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. > > > > > > yes, it caused me to pause to but it was the simplest way i saw to do > > > the check > > > > > > > > > > > > > > +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. > > > > > > If there is "real, demonstrable evidence that this happens in real-world > > > files" then we would likely have a sample and not ask for one with > > > avpriv_request_sample() > > > > > > I think its very plausible that a encoder would use a weight that is > > > outside the range. Printing something does make sense. > > > > i will apply this with the label chaned to out_range_weight: > > unless there are objections > > > OK. applied thanks [...]
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) {
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@niedermayer.cc> --- libavcodec/h264_parse.c | 9 +++++++++ libavcodec/h264_slice.c | 7 +++++-- 2 files changed, 14 insertions(+), 2 deletions(-)