| Message ID | 20170917004211.12545-1-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Sat, Sep 16, 2017 at 8:42 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > + if (get_bits_left(&s->gb) < 0) { > + av_log(s->avctx, AV_LOG_ERROR, > + "Insufficient bits left at %d %d\n", s->mb_x, s->mb_y); > + return AVERROR_INVALIDDATA; > + } We've talked about this before, av_log(AV_LOG_ERROR) is not appropriate for such terse and unhelpful messages that really only apply to fuzz-broken files... Ronald
Hi On Sun, Sep 17, 2017 at 04:16:13PM -0400, Ronald S. Bultje wrote: > Hi, > > On Sat, Sep 16, 2017 at 8:42 PM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > + if (get_bits_left(&s->gb) < 0) { > > + av_log(s->avctx, AV_LOG_ERROR, > > + "Insufficient bits left at %d %d\n", s->mb_x, s->mb_y); > > + return AVERROR_INVALIDDATA; > > + } > > > We've talked about this before, av_log(AV_LOG_ERROR) is not appropriate for > such terse and unhelpful messages that really only apply to fuzz-broken > files... We talked about this before. And we disagree on almost everything certainly the implied assumptions you repeat here. Also i must say i really would prefer to work on the code without these debates. I maintain the code in question here, can people not just let me maintain the code ... IIRC, previously you wanted "All" these error messages removed from the binary. Your statement above now leaves it more wide open how to resolve it But if you still want them removed from the binary, then my reply is still the same Iam happy to follow what the community prefers. I have seen no evidence that theres a majority preferring to remove all error messages for errors that have been found by help of automated fuzzers. Ive seen 3 or 4 people complaining about error messages and ive seen people working on fixing security issues (not just me) disagreeing when they where told to remove messages. I may be wrong but i suspect the "hardline" total removal has few people supporting it. Especially outside FFmpeg and the projects surrounding it. Ive not seen this radical viewpoint elsewhere. Detailed error messages are valuable If OTOH what you suggest now (you did after all not state it clearly in your reply) is litterally just changing away from a terse av_log(AV_LOG_ERROR) message. I certainly can try to improve the error message, make it clearer and more verbose. Thanks [...]
Hi Michael, On Sun, Sep 17, 2017 at 8:15 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > Iam happy to follow what the community prefers. > Some don't like it, some don't care. I think everyone would be happy (and thus the sum of happiness would increase) if you changed this to ff_dlog() or something along those lines. You say you want to code, so why not take the path of least resistance and move on? Is this just about being right? Or do you really believe it's important to display an error message while fuzzing? Or do you have actual evidence that this is an error path that will often occur in real-world files and where the provided error message helps our users resolve the issue that their valid (non-fuzzed, real-world) file is not playing back? I don't understand. Ronald
2017-09-18 15:11 GMT+02:00 Ronald S. Bultje <rsbultje@gmail.com>: > Hi Michael, > > On Sun, Sep 17, 2017 at 8:15 PM, Michael Niedermayer <michael@niedermayer.cc >> wrote: > >> Iam happy to follow what the community prefers. >> > > Some don't like it, some don't care. I think everyone would be happy (and > thus the sum of happiness would increase) if you changed this to ff_dlog() > or something along those lines. I don't think this makes much sense for an error message shown on corrupted input. Carl Eugen
On 9/18/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-09-18 15:11 GMT+02:00 Ronald S. Bultje <rsbultje@gmail.com>: >> Hi Michael, >> >> On Sun, Sep 17, 2017 at 8:15 PM, Michael Niedermayer >> <michael@niedermayer.cc >>> wrote: >> >>> Iam happy to follow what the community prefers. >>> >> >> Some don't like it, some don't care. I think everyone would be happy (and >> thus the sum of happiness would increase) if you changed this to ff_dlog() >> or something along those lines. > > I don't think this makes much sense for an error message shown > on corrupted input. Such messages just pollute codebase and binary.
Hi Ronald On Mon, Sep 18, 2017 at 09:11:39AM -0400, Ronald S. Bultje wrote: > Hi Michael, > > On Sun, Sep 17, 2017 at 8:15 PM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > Iam happy to follow what the community prefers. > > > > Some don't like it, some don't care. I think everyone would be happy (and I belive you did not ask the community. if so you cannot know what people prefer. I think you extrapolate from 3 people who have been very vocal about this to the whole community. > thus the sum of happiness would increase) if you changed this to ff_dlog() > or something along those lines. Why do you keep arguing with me and not poll the community? Iam happy to do what the majority prefers, but i do not know what the majority prefers. > > You say you want to code, so why not take the path of least resistance and > move on? If this is about changing one error message in one patch. Ill just change it because id like to push this bugfix and argumentation with you is quite time consuming and was so far in this case fruitless. But the project should not be run by "its the path of least resistance to comply with what i demand" if OTOH what you want is establishing a rule about error messages. No, one cannot do that without 1. writing the rule down 2. polling the comunity about it 3. including the rule in the policy so everyone, including new developers know about it. This is a matter that needs a community decission first. Also the codebase would become incresingly inconsistent without this being agreed on by all and written down. > Is this just about being right? no, its very possible i and or you are wrong. > Or do you really believe it's > important to display an error message while fuzzing? no. Its important when the user hits a bug/issue. Developers can rebuild the tree with any debug flag, a user generally cannot. But i belive we fundamentally disagree here. The ones hit hardest with a wide spread removial of error messages from the binary in a user build are probably the people dealing with bug reports. Its interresting to note that the people pushing for this removial have quite some overlap with who has in the past attacked our most active developer on the bug tracker. Iam sure its just a coincidence but its still chilling to realize this. > Or do you have actual > evidence that this is an error path that will often occur in real-world > files and where the provided error message helps our users resolve the > issue that their valid (non-fuzzed, real-world) file is not playing back? That yes, though its a very seperate thing. And part of what i meant with "disagreeing" with you. This error message here would trigger on a truncated frame/file. Truncated files are common in the real world for all kinds of reasons. This was found by a fuzzer but its not specific to fuzzing Also, i very much would like to end this discussion, its a huge waste of time for everyone. If you dont want to have a community wide rule and dont want to ask people outside of the group who dont like how error messages are currently handled, then please stop asking me to change code. Thank you [...]
diff --git a/libavcodec/wmv2dec.c b/libavcodec/wmv2dec.c index 20dbee5703..62291d4163 100644 --- a/libavcodec/wmv2dec.c +++ b/libavcodec/wmv2dec.c @@ -30,7 +30,7 @@ #include "wmv2.h" -static void parse_mb_skip(Wmv2Context *w) +static int parse_mb_skip(Wmv2Context *w) { int mb_x, mb_y; MpegEncContext *const s = &w->s; @@ -45,6 +45,8 @@ static void parse_mb_skip(Wmv2Context *w) MB_TYPE_16x16 | MB_TYPE_L0; break; case SKIP_TYPE_MPEG: + if (get_bits_left(&s->gb) < s->mb_height * s->mb_width) + return AVERROR_INVALIDDATA; for (mb_y = 0; mb_y < s->mb_height; mb_y++) for (mb_x = 0; mb_x < s->mb_width; mb_x++) mb_type[mb_y * s->mb_stride + mb_x] = @@ -52,6 +54,8 @@ static void parse_mb_skip(Wmv2Context *w) break; case SKIP_TYPE_ROW: for (mb_y = 0; mb_y < s->mb_height; mb_y++) { + if (get_bits_left(&s->gb) < 1) + return AVERROR_INVALIDDATA; if (get_bits1(&s->gb)) { for (mb_x = 0; mb_x < s->mb_width; mb_x++) mb_type[mb_y * s->mb_stride + mb_x] = @@ -65,6 +69,8 @@ static void parse_mb_skip(Wmv2Context *w) break; case SKIP_TYPE_COL: for (mb_x = 0; mb_x < s->mb_width; mb_x++) { + if (get_bits_left(&s->gb) < 1) + return AVERROR_INVALIDDATA; if (get_bits1(&s->gb)) { for (mb_y = 0; mb_y < s->mb_height; mb_y++) mb_type[mb_y * s->mb_stride + mb_x] = @@ -77,6 +83,7 @@ static void parse_mb_skip(Wmv2Context *w) } break; } + return 0; } static int decode_ext_header(Wmv2Context *w) @@ -170,9 +177,12 @@ int ff_wmv2_decode_secondary_picture_header(MpegEncContext *s) } } else { int cbp_index; + int ret; w->j_type = 0; - parse_mb_skip(w); + ret = parse_mb_skip(w); + if (ret < 0) + return ret; cbp_index = decode012(&s->gb); w->cbp_table_index = wmv2_get_cbp_table_index(s, cbp_index); @@ -345,6 +355,12 @@ int ff_wmv2_decode_mb(MpegEncContext *s, int16_t block[6][64]) if (w->j_type) return 0; + if (get_bits_left(&s->gb) < 0) { + av_log(s->avctx, AV_LOG_ERROR, + "Insufficient bits left at %d %d\n", s->mb_x, s->mb_y); + return AVERROR_INVALIDDATA; + } + if (s->pict_type == AV_PICTURE_TYPE_P) { if (IS_SKIP(s->current_picture.mb_type[s->mb_y * s->mb_stride + s->mb_x])) { /* skip mb */
Fixes: Timeout Fixes: 3200/clusterfuzz-testcase-5750022136135680 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/wmv2dec.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)