[FFmpeg-devel] [PATCH] avcodec/snowdec: Check ld/cbd/crd

[Michael Niedermayer]

On Sun, Sep 03, 2017 at 11:43:54AM -0300, James Almer wrote:
> On 9/3/2017 10:49 AM, Ronald S. Bultje wrote:
> > Hi,
> > 
> > On Sun, Sep 3, 2017 at 6:23 AM, Michael Niedermayer <michael at niedermayer.cc>
> > wrote:
> > 
> >> Fixes: Timeout
> >> Fixes: 3142/clusterfuzz-testcase-5007853163118592
> >>
> >> Found-by: continuous fuzzing process https://github.com/google/oss-
> >> fuzz/tree/master/projects/ffmpeg
> >> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> >> ---
> >>  libavcodec/snowdec.c | 19 +++++++++++++++----
> >>  1 file changed, 15 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c
> >> index b74c468ce3..7e07857a44 100644
> >> --- a/libavcodec/snowdec.c
> >> +++ b/libavcodec/snowdec.c
> >> @@ -183,13 +183,24 @@ static int decode_q_branch(SnowContext *s, int
> >> level, int x, int y){
> >>          int my_context= av_log2(2*FFABS(left->my - top->my)) +
> >> 0*av_log2(2*FFABS(tr->my - top->my));
> >>
> >>          type= get_rac(&s->c, &s->block_state[1 + left->type + top->type])
> >> ? BLOCK_INTRA : 0;
> >> -
> >>          if(type){
> >> +            int ld, cbd, crd;
> >>              pred_mv(s, &mx, &my, 0, left, top, tr);
> >> -            l += get_symbol(&s->c, &s->block_state[32], 1);
> >> +            ld = get_symbol(&s->c, &s->block_state[32], 1);
> >> +            if (ld < -255 || ld > 255) {
> >> +                av_log(s->avctx, AV_LOG_ERROR, "Invalid ld %d\n", ld);
> >> +                return AVERROR_INVALIDDATA;
> >> +            }
> >> +            l += ld;
> >>              if (s->nb_planes > 2) {
> >> -                cb+= get_symbol(&s->c, &s->block_state[64], 1);
> >> -                cr+= get_symbol(&s->c, &s->block_state[96], 1);
> >> +                cbd = get_symbol(&s->c, &s->block_state[64], 1);
> >> +                crd = get_symbol(&s->c, &s->block_state[96], 1);
> >> +                if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) {
> >> +                    av_log(s->avctx, AV_LOG_ERROR, "Invalid cbd %d, crd
> >> %d\n", cbd, crd);
> >> +                    return AVERROR_INVALIDDATA;
> >> +                }
> >> +                cb += cbd;
> >> +                cr += crd;
> >>              }
> > 
> > 
> > Can you elaborate on how these error messages, which are displayed to the
> > user by default, help the user resolve the
> > likely-to-occur-with-realworld-files situation where a validly-created file
> > doesn't play back?
> > 
> > If any part of this sentence is not true, then why is there a message here?
> > 
> > Ronald
> 
> Just go straight to the point, please. This fuzzing commit set in the
> past few months has been way more controversial than it has any right to.
> 
> Michael: Don't add error messages at any level above debug if they are
> completely useless and unhelpful for non-developers. And Consider using

We have always printed error messages for the case that an error
occured.
Its unprofessional to fail decoding a file but not provide any
hint as to why a failure occured.

If we remove all error messages and just print a generic "failed
decoding header" or "failed to decode frame"
We would leave users wondering about each error and we would no longer
have differentiated bug reports.
There would be one very huge bug report about
"Error while decoding stream #0:0: Invalid data found when processing input"
Because thats all detail the user can get if the message is not in the
binary.
That bug report then would be marked invalid of course and would help
neither users nor developers.
Maybe it would be one such report per codec but thats still rather
useless.
Or there would be one report for every single file with dozends of
duplicates, again not what we want.
We want the user to do most of the bug reporting work not use the
limited developer manpower.

Iam not sure why error messages are since about a year or so
considered controversial by some developers but not before 

And why this is directed toward some types of changes and some parts of
the codebase but not others.

You can look at ronalds vp9*c
It containas in fact more error messages than snow does.
git grep AV_LOG_ERROR libavcodec/vp9*c
vs.
git grep AV_LOG_ERROR libavcodec/snow*c

These error messages do not substantially differ in what they are
about. Its values being out of supported or valid range, allocation
failures, and so forth. Same as also this patch here

If the community as a whole prefers not to add detailed error messages
and just print something like the current generic
"Error while decoding stream #0:0: Invalid data found when processing input"
Then this should be properly agreed upon and documeted and enforced in
everyones code and not just 10% of the changes.
Also we may in this case need a team of people doing "bug report
analysis and sorting" as users can then no longer group/guess
issues similarity based on detailed error messages.

It also makes debuging harder as there is less information initially
available in a report.

And users hitting an issue also cant google with the error message
and for example find that it was fixed in a newer version


> ff_tlog() as well, so they don't become binary bloat on a non-debug build.

I belive english text is negligible size wise in the binaries
am i missing something ?
Our code is already quite terse and devoid of comments and
documentation, error messages do not seem a exception here

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"Nothing to hide" only works if the folks in power share the values of
you and everyone you know entirely and always will -- Tom Scott

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170904/8780f05b/attachment.sig>