| Message ID | 20190722215724.10386-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 965e766e4892cfc45c97cca88895248a7735e7d0 |
| Headers | show |
Jul 22, 2019, 10:57 PM by michael@niedermayer.cc: > The dimensions are always 320x200 they are hardcoded in the demuxer. > Hardcode them instead in the decoder. > > Fixes: Timeout (16sec -> 400ms) > Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/rl2.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c > index 6662979c52..2d336a61e5 100644 > --- a/libavcodec/rl2.c > +++ b/libavcodec/rl2.c > @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext *avctx) > Rl2Context *s = avctx->priv_data; > int back_size; > int i; > + int ret; > > s->avctx = avctx; > avctx->pix_fmt = AV_PIX_FMT_PAL8; > > + ret = ff_set_dimensions(avctx, 320, 200); > + if (ret < 0) > + return ret; > + > /** parse extra data */ > if (!avctx->extradata || avctx->extradata_size < EXTRADATA1_SIZE) { > av_log(avctx, AV_LOG_ERROR, "invalid extradata size\n"); > -- > 2.22.0 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org>> with subject "unsubscribe". > If the size somehow gets lost between lavf and lavc then the problem is there, and should be solved by not fudging the decoder. Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different? Please, stop it with those patches. It takes energy and some worry to have to open up the ML and scan it for bad patches from people who really should know better every time.
Am Di., 23. Juli 2019 um 00:14 Uhr schrieb Lynne <dev@lynne.ee>: > If the size somehow gets lost between lavf and lavc then the problem is there, and should be solved by not fudging the decoder. (Using libavformat is no condition for using libavcodec) > Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different? > Please, stop it with those patches. Please stop with these comments. No matter what others may tell you, they are not useful but very disturbing. Carl Eugen
On Tue, Jul 23, 2019 at 12:13:51AM +0200, Lynne wrote: > Jul 22, 2019, 10:57 PM by michael@niedermayer.cc: > > > The dimensions are always 320x200 they are hardcoded in the demuxer. > > Hardcode them instead in the decoder. > > > > Fixes: Timeout (16sec -> 400ms) > > Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/rl2.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c > > index 6662979c52..2d336a61e5 100644 > > --- a/libavcodec/rl2.c > > +++ b/libavcodec/rl2.c > > @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext *avctx) > > Rl2Context *s = avctx->priv_data; > > int back_size; > > int i; > > + int ret; > > > > s->avctx = avctx; > > avctx->pix_fmt = AV_PIX_FMT_PAL8; > > > > + ret = ff_set_dimensions(avctx, 320, 200); > > + if (ret < 0) > > + return ret; > > + > > /** parse extra data */ > > if (!avctx->extradata || avctx->extradata_size < EXTRADATA1_SIZE) { > > av_log(avctx, AV_LOG_ERROR, "invalid extradata size\n"); > > -- > > 2.22.0 > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org>> with subject "unsubscribe". > > > > If the size somehow gets lost between lavf and lavc then the problem is there, and should be solved by not fudging the decoder. libavcodec is used by many applications which do not use libavformat. for all i know and what is documented in the only reference spec i have. the resolution is a constant its always 320x200 for this codec. So its natural to have the decoder know about its own resolution. > Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different? > Please, stop it with those patches. It takes energy and some worry to have to open up the ML and scan it for bad patches from people who really should know better every time. Please stop with these attacks, this is just making people become mutually more aggressive and filled with hatred. Lets work together to find solutions, not work against each other. Thanks [...]
Jul 23, 2019, 12:00 AM by michael@niedermayer.cc: > On Tue, Jul 23, 2019 at 12:13:51AM +0200, Lynne wrote: > >> Jul 22, 2019, 10:57 PM by michael@niedermayer.cc: >> >> > The dimensions are always 320x200 they are hardcoded in the demuxer. >> > Hardcode them instead in the decoder. >> > >> > Fixes: Timeout (16sec -> 400ms) >> > Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712 >> > >> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > --- >> > libavcodec/rl2.c | 5 +++++ >> > 1 file changed, 5 insertions(+) >> > >> > diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c >> > index 6662979c52..2d336a61e5 100644 >> > --- a/libavcodec/rl2.c >> > +++ b/libavcodec/rl2.c >> > @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext *avctx) >> > Rl2Context *s = avctx->priv_data; >> > int back_size; >> > int i; >> > + int ret; >> > >> > s->avctx = avctx; >> > avctx->pix_fmt = AV_PIX_FMT_PAL8; >> > >> > + ret = ff_set_dimensions(avctx, 320, 200); >> > + if (ret < 0) >> > + return ret; >> > + >> > /** parse extra data */ >> > if (!avctx->extradata || avctx->extradata_size < EXTRADATA1_SIZE) { >> > av_log(avctx, AV_LOG_ERROR, "invalid extradata size\n"); >> > -- >> > 2.22.0 >> > >> > _______________________________________________ >> > ffmpeg-devel mailing list >> > ffmpeg-devel@ffmpeg.org >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > >> > To unsubscribe, visit link above, or email >> > ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org>> with subject "unsubscribe". >> > >> >> If the size somehow gets lost between lavf and lavc then the problem is there, and should be solved by not fudging the decoder. >> > > libavcodec is used by many applications which do not use libavformat. > for all i know and what is documented in the only reference spec i have. > the resolution is a constant its always 320x200 for this codec. So its > natural to have the decoder know about its own resolution. > Nonetheless, while I agree with having both decoder and demuxer hardcode the resolution, I still think this is fudging. >> Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different? >> > > >> Please, stop it with those patches. It takes energy and some worry to have to open up the ML and scan it for bad patches from people who really should know better every time. >> > > Please stop with these attacks, this is just making people become > mutually more aggressive and filled with hatred. > Of course I'm frustrated, I've been ignored. These patches affect decoding of real world broken files in favor of fixing specially crafted fuzzed files. Sure, protecting against ddos attacts is important, but not important enough to make decoders give up early and return nothing. Especially in cases where the timeout speedup is of the order of 2s to 400ms. Yet in all of those timeout patches all you've cared about is shutting up the tool. You've never once shown any figures if this could affect decoding, because its a lot harder than just showing they fix something some tool calls a timeout and forgetting about it. You haven't even commented on this when I asked you on IRC. You also sneak this type of patches in when there's an overflow later on during decoding, which is completely incorrect in almost all cases, for the same reason above. If people really wanted to ddos anything using decoders they can easly do so by sending over a gzip compressed specially crafted vobis file to allocate tons of memory and spend CPU. Something every browser supports, rather than use old codecs disabled by everyone.
Jul 22, 2019, 11:24 PM by ceffmpeg@gmail.com: > Am Di., 23. Juli 2019 um 00:14 Uhr schrieb Lynne <dev@lynne.ee>: > >> If the size somehow gets lost between lavf and lavc then the problem is there, and should be solved by not fudging the decoder. >> > > (Using libavformat is no condition for using libavcodec) > >> Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different? >> >> Please, stop it with those patches. >> > > Please stop with these comments. > > No matter what others may tell you, they are not useful but very disturbing. > Please stay out of all my emails and harassing me over apparently not having enough commits.
Lynne (12019-07-23): > Please stay out of all my emails and harassing me over apparently not > having enough commits. What are you taking about? I have gone over your interactions on the mailing-list, and found nothing about number of commits, nor anything that looks to me like harassment, but I may have missed something. Can you elaborate? Regards,
Am Di., 23. Juli 2019 um 04:47 Uhr schrieb Lynne <dev@lynne.ee>: > > Jul 22, 2019, 11:24 PM by ceffmpeg@gmail.com: > > Please stop with these comments. > > > > No matter what others may tell you, they are not useful but very disturbing. > > Please stay out of all my emails I wish I could! Sadly, you keep attacking FFmpeg developers and sending these attacks to a public mailing list on which I happen to be active. Carl Eugen
On 7/23/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Am Di., 23. Juli 2019 um 04:47 Uhr schrieb Lynne <dev@lynne.ee>: >> >> Jul 22, 2019, 11:24 PM by ceffmpeg@gmail.com: > >> > Please stop with these comments. >> > >> > No matter what others may tell you, they are not useful but very >> > disturbing. >> >> Please stay out of all my emails > > I wish I could! > Sadly, you keep attacking FFmpeg developers and sending these attacks to a > public mailing list on which I happen to be active. FFmpeg developer attacking other FFmpeg developers? This can't be true.
Jul 23, 2019, 9:13 AM by george@nsup.org: > Lynne (12019-07-23): > >> Please stay out of all my emails and harassing me over apparently not >> having enough commits. >> > > What are you taking about? I have gone over your interactions on the > mailing-list, and found nothing about number of commits, nor anything > that looks to me like harassment, but I may have missed something. Can > you elaborate? > IRC.
On 22.07.2019, at 23:57, Michael Niedermayer <michael@niedermayer.cc> wrote: > The dimensions are always 320x200 they are hardcoded in the demuxer. > Hardcode them instead in the decoder. > > Fixes: Timeout (16sec -> 400ms) > Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/rl2.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c > index 6662979c52..2d336a61e5 100644 > --- a/libavcodec/rl2.c > +++ b/libavcodec/rl2.c > @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext *avctx) > Rl2Context *s = avctx->priv_data; > int back_size; > int i; > + int ret; > > s->avctx = avctx; > avctx->pix_fmt = AV_PIX_FMT_PAL8; > > + ret = ff_set_dimensions(avctx, 320, 200); > + if (ret < 0) > + return ret; I really dislike these completely comment-less patches. So it seems this is only based on what our demuxer does. However does the format itself have any inherent size limitations? What was the cause of the slow decoding? Does this actually fix it, or does it just swipe it "under the carpet"? If someone ever found a sample with a different solution, how would they know that they shouldn't just remove this again? Without any kind of comment on the point of this call, people might assume it's pointless nonsense.
> > What was the cause of the slow decoding? Does this actually fix it, or > does it just swipe it "under the carpet"? > If someone ever found a sample with a different solution, how would they > know that they shouldn't just remove this again? Without any kind of > comment on the point of this call, people might assume it's pointless > nonsense. > A significant proportion of these patches sweep the issue under the carpet. Not to mention the swathes of annoyed developers Kieran
On 24.07.2019, at 02:39, Kieran Kunhya <kierank@obe.tv> wrote: >> >> What was the cause of the slow decoding? Does this actually fix it, or >> does it just swipe it "under the carpet"? >> If someone ever found a sample with a different solution, how would they >> know that they shouldn't just remove this again? Without any kind of >> comment on the point of this call, people might assume it's pointless >> nonsense. >> > > A significant proportion of these patches sweep the issue under the carpet. Which is not necessarily the wrong choice. But by leaving no documentation a lot of the time spend on writing the patches is wasted since the knowledge gained is just lost, maintainers need to "reverse-engineer" them etc. > Not to mention the swathes of annoyed developers And let's not turn this into a conflict, I just tried to give clear feedback why I am unhappy with some of these patches even though they may be worthwhile still. > Hopefully some of that also captures reasons others feel unhappy (and it would be nice if the concerns could be raised more constructively, though I do acknowledge it can be hard).
On Wed, Jul 24, 2019 at 01:29:26AM +0200, Reimar Döffinger wrote: > > > On 22.07.2019, at 23:57, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > The dimensions are always 320x200 they are hardcoded in the demuxer. > > Hardcode them instead in the decoder. > > > > Fixes: Timeout (16sec -> 400ms) > > Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/rl2.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c > > index 6662979c52..2d336a61e5 100644 > > --- a/libavcodec/rl2.c > > +++ b/libavcodec/rl2.c > > @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext *avctx) > > Rl2Context *s = avctx->priv_data; > > int back_size; > > int i; > > + int ret; > > > > s->avctx = avctx; > > avctx->pix_fmt = AV_PIX_FMT_PAL8; > > > > + ret = ff_set_dimensions(avctx, 320, 200); > > + if (ret < 0) > > + return ret; > > I really dislike these completely comment-less patches. > So it seems this is only based on what our demuxer does. No, the only specification available to me: https://wiki.multimedia.cx/index.php/RL2 says this: "Video Decoding The video is always encoded in 320x200 resolution." I will add this to the commit message of course in case no other objections remain > However does the format itself have any inherent size limitations? yes, it conatins a video_base parameter which is 16bit and indexes into the width*height array of pixels. suggesting that a size around 320x200 is the intended maximum which would use that parameter. > What was the cause of the slow decoding? Does this actually fix it, or does it just swipe it "under the carpet"? The cause of slow decoding was a huge input resolution, precissely 64768x255 that also is far from the worst possible > If someone ever found a sample with a different solution, how would they know that they shouldn't just remove this again? Without any kind of comment on the point of this call, people might assume it's pointless nonsense. if some rl2 "2.0" is found which does store a resolution and if that is reverse engeneered (these being obviously pre-requirements) then the removial of the hardcoded size is probably not completely unreasonable Thanks [...]
On Wed, Jul 24, 2019 at 01:39:01AM +0100, Kieran Kunhya wrote: > > > > What was the cause of the slow decoding? Does this actually fix it, or > > does it just swipe it "under the carpet"? > > If someone ever found a sample with a different solution, how would they > > know that they shouldn't just remove this again? Without any kind of > > comment on the point of this call, people might assume it's pointless > > nonsense. > > > > A significant proportion of these patches sweep the issue under the carpet. If you know of a problem with a patch and or if you have a suggestion for improving one, report the issue to the author of the patch. Such general statments are not helping the project because noone can fix a wide unspecific claim like that or even verify if the statement is true. Which makes this just an ad hominem attack. Because it can very easily be assigned to a human (who wrote all the patches) while making it impossible to assign it to a technical issue that can be corrected or discussed. Thanks [...]
On Tue, Jul 23, 2019 at 04:42:32AM +0200, Lynne wrote: > Jul 23, 2019, 12:00 AM by michael@niedermayer.cc: > > > On Tue, Jul 23, 2019 at 12:13:51AM +0200, Lynne wrote: > > > >> Jul 22, 2019, 10:57 PM by michael@niedermayer.cc: > >> > >> > The dimensions are always 320x200 they are hardcoded in the demuxer. > >> > Hardcode them instead in the decoder. > >> > > >> > Fixes: Timeout (16sec -> 400ms) > >> > Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712 > >> > > >> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> > --- > >> > libavcodec/rl2.c | 5 +++++ > >> > 1 file changed, 5 insertions(+) > >> > > >> > diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c > >> > index 6662979c52..2d336a61e5 100644 > >> > --- a/libavcodec/rl2.c > >> > +++ b/libavcodec/rl2.c > >> > @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext *avctx) > >> > Rl2Context *s = avctx->priv_data; > >> > int back_size; > >> > int i; > >> > + int ret; > >> > > >> > s->avctx = avctx; > >> > avctx->pix_fmt = AV_PIX_FMT_PAL8; > >> > > >> > + ret = ff_set_dimensions(avctx, 320, 200); > >> > + if (ret < 0) > >> > + return ret; > >> > + > >> > /** parse extra data */ > >> > if (!avctx->extradata || avctx->extradata_size < EXTRADATA1_SIZE) { > >> > av_log(avctx, AV_LOG_ERROR, "invalid extradata size\n"); > >> > -- > >> > 2.22.0 > >> > > >> > _______________________________________________ > >> > ffmpeg-devel mailing list > >> > ffmpeg-devel@ffmpeg.org > >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > > >> > To unsubscribe, visit link above, or email > >> > ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org>> with subject "unsubscribe". > >> > > >> > >> If the size somehow gets lost between lavf and lavc then the problem is there, and should be solved by not fudging the decoder. > >> > > > > libavcodec is used by many applications which do not use libavformat. > > for all i know and what is documented in the only reference spec i have. > > the resolution is a constant its always 320x200 for this codec. So its > > natural to have the decoder know about its own resolution. > > > > Nonetheless, while I agree with having both decoder and demuxer hardcode the resolution, I still think this is fudging. > > > > >> Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different? > >> > > > > > >> Please, stop it with those patches. It takes energy and some worry to have to open up the ML and scan it for bad patches from people who really should know better every time. > >> > > > > Please stop with these attacks, this is just making people become > > mutually more aggressive and filled with hatred. > > > > Of course I'm frustrated, I've been ignored. What did you expect ? IIRC you have asked for whole classes of security issues to be not fixed. Something like that would require a vote and majority of developers. > These patches affect decoding of real world broken files in favor of fixing specially crafted fuzzed files. Iam happy to look into such cases. Can you provide me with such "real world broken files"? Its not intended to worsen the output from such files > Sure, protecting against ddos attacts is important, but not important enough to make decoders give up early and return nothing. Especially in cases where the timeout speedup is of the order of 2s to 400ms. > Yet in all of those timeout patches all you've cared about is shutting up the tool. You've never once shown any figures if this could affect decoding, because its a lot harder than just showing they fix something some tool calls a timeout and forgetting about it. You haven't even commented on this when I asked you on IRC. > You also sneak this type of patches in when there's an overflow later on during decoding, which is completely incorrect in almost all cases, for the same reason above. if you know of issues in a patch or commit you should report this during patch review or as soon as you find out about the issue as a reply to that patch or commit or as mail to the author. So the author or any other developer can fix it or the patch is on hold until its fixed. Complaining out of context about unspecified issues. Or well IRC (i dont know what you refer to here at all either) Is just guranteed to lead to frustration because that wont and cant lead to issues being fixed PS: Again, i strongly suggest we work together to find acceptable solutions to these issues. Because that would lead to good solutions, and everyone being non frustrated. When one demands "dont fix it" that is what leads to a fight and frustration and a blocked patch so really both sides are unhappy and frustrated Thanks [...]
Jul 24, 2019, 11:08 AM by michael@niedermayer.cc: > > What did you expect ? IIRC you have asked for whole classes of security > issues to be not fixed. > > Something like that would require a vote and majority of developers. > The way I interpret this: "Of course I ignored you, you're mental!", doesn't help. And I don't think its just me. And you do remember incorrectly in saying that I want this whole class of security issues not fixed. In this thread I specifically raised the issue of what is considered to be a security issue by asking whether a speedup of failing to decode from 2 to 0.4 seconds is considered such or what's considered acceptable in general. And I think I'll disagree that it is. 16 seconds to 2 seconds I can accept, but not 2 to 0.4. >> These patches affect decoding of real world broken files in favor of fixing specially crafted fuzzed files. >> > > Iam happy to look into such cases. Can you provide me with such > "real world broken files"? > Its not intended to worsen the output from such files > Simple logical analysis, "if file is somewhat broken, don't try decoding" does very well indicate that it won't only apply for _this_ broken file, but in general. Thus, this is for you to prove. I've said it before that otherwise its a burden to other developers to have to screen all of these patches. >> Sure, protecting against ddos attacts is important, but not important enough to make decoders give up early and return nothing. Especially in cases where the timeout speedup is of the order of 2s to 400ms. >> Yet in all of those timeout patches all you've cared about is shutting up the tool. You've never once shown any figures if this could affect decoding, because its a lot harder than just showing they fix something some tool calls a timeout and forgetting about it. You haven't even commented on this when I asked you on IRC. >> You also sneak this type of patches in when there's an overflow later on during decoding, which is completely incorrect in almost all cases, for the same reason above. >> > > if you know of issues in a patch or commit you should report this > during patch review or as soon as you find out about the issue > as a reply to that patch or commit or as mail to the author. > That's what I'm doing. That aside, you've completely ignored my statements on what's considered acceptable, showing figures, and sneaking this type of patches to fix undefined behavior. Making your reply a simple refutation, rather than addressing anything I've said. So I'm asking you again, what is considered a security issue and what is considered acceptable? And what is considered not a security issue but a complaint from an overzealous automated tool. > So the author or any other developer can fix it or the patch is on hold > until its fixed. > Is not commiting it at all because there is no issue, adds unnecessary code which makes things worse, only to speed failing to decode by milliseconds for a very specific crafted input to silence an automated tool completely not an option? > Complaining out of context about unspecified issues. Or well IRC (i dont > know what you refer to here at all either) > Is just guranteed to lead to frustration because that wont and cant lead to > issues being fixed > I am complaining in the context of this thread about specific issues that I'm worried all of these patches have, and I'm not alone, despite you trying to label someone else's concerned reply as a personal attack. I'm sure other developers would agree if asked, just probably not like this in a situation where a reply is labelled as an attack. > PS: Again, i strongly suggest we work together to find acceptable solutions > to these issues. Because that would lead to good solutions, and everyone > being non frustrated. When one demands "dont fix it" that is what leads > to a fight and frustration and a blocked patch so really both sides are > unhappy and frustrated > We are. Or I'm trying to, but being simply refuted only goes so far.
On 7/23/2019 9:39 PM, Kieran Kunhya wrote: >> >> What was the cause of the slow decoding? Does this actually fix it, or >> does it just swipe it "under the carpet"? >> If someone ever found a sample with a different solution, how would they >> know that they shouldn't just remove this again? Without any kind of >> comment on the point of this call, people might assume it's pointless >> nonsense. >> > > A significant proportion of these patches sweep the issue under the carpet. > Not to mention the swathes of annoyed developers > > Kieran The only very vocal people so far that i could see have been you and Lynne. Everyone else at most didn't find these patches useful, but Lynne especially has been very aggressive to communicate their displeasure about fuzzing related fixes, and unjustifiably so. If some of these patches affect code you're interested into, then suggest nicer looking alternatives, like i did for the recent mpc8 patch. Patches like this one that fixes timeouts during fuzzing are usually validation checks to abort on invalid data early, and in this case they actually force an spec constrain in the correct place. They are hardly clutter. And if they use what you consider magic numbers or heuristics, or "sweep the issue under the carpet", you can always just request a comment to be placed next to them, like Reimar just did, or use some configurable value, like the recently added AVCodecContext.discard_damaged_percentage My point is, instead of complaining about UB fixes, try to be constructive about how to implement them better. Campaigning to migrate the codebase to something else than C and cut the issue from the root is more productive than replying to every other fuzzing fix like this.
On 7/24/19, James Almer <jamrial@gmail.com> wrote: > On 7/23/2019 9:39 PM, Kieran Kunhya wrote: >>> >>> What was the cause of the slow decoding? Does this actually fix it, or >>> does it just swipe it "under the carpet"? >>> If someone ever found a sample with a different solution, how would they >>> know that they shouldn't just remove this again? Without any kind of >>> comment on the point of this call, people might assume it's pointless >>> nonsense. >>> >> >> A significant proportion of these patches sweep the issue under the >> carpet. >> Not to mention the swathes of annoyed developers >> >> Kieran > > The only very vocal people so far that i could see have been you and > Lynne. Everyone else at most didn't find these patches useful, but Lynne > especially has been very aggressive to communicate their displeasure > about fuzzing related fixes, and unjustifiably so. > If some of these patches affect code you're interested into, then > suggest nicer looking alternatives, like i did for the recent mpc8 patch. > > Patches like this one that fixes timeouts during fuzzing are usually > validation checks to abort on invalid data early, and in this case they > actually force an spec constrain in the correct place. They are hardly > clutter. And if they use what you consider magic numbers or heuristics, > or "sweep the issue under the carpet", you can always just request a > comment to be placed next to them, like Reimar just did, or use some > configurable value, like the recently added > AVCodecContext.discard_damaged_percentage > > My point is, instead of complaining about UB fixes, try to be > constructive about how to implement them better. Campaigning to migrate > the codebase to something else than C and cut the issue from the root is > more productive than replying to every other fuzzing fix like this. This are not UB fixes, this are bandaids for some cases of too big frame width & height sizes given as invalid input to decoder. Instead of adding broken heuristic for every and each decoder one should simply limit video sizes which are being fuzzed.
Lynne (12019-07-23):
> IRC.
Each time I ask where something horrible that causes a flamewar between
developers had been said, I get the same answer: IRC.
I am starting to believe that the problem is IRC.
Has IRC ever been useful for the development of FFmpeg?
I do not know if it has, but I am pretty sure that when the discussion
starts to become unhelpful, IRC makes it ten time worse. The need for
short and fast answers, the lack of threading, the mixed discussions,
the synchronicity: all that make it hard to express things with
subtlety, and makes the bad bits stand out.
For what it is worth, I have observed the same thing on Twitter, for the
same reasons.
Therefore, I would like to give that advice to everybody who hang on
IRC: Whenever you see something hurtful, do not take it personally, say
"STOP" and move the discussion to the mailing list.
And when on the mailing-list, remember: it is not IRC. You are not
required to reply in less than a minute and less than two lines. Take
your time, express your ideas and disagreements with accuracy and
details. It will eventually save time, for you and for everybody.
As for the current issue, I think it would be better if objections, when
they are not about a precise technical issue ("this patch breaks
decoding of file X"), were constructive.
In other words, tell us not only why you dislike these patches, but what
you would do about the issues they fix instead.
Otherwise, it seems like you are despising other people's efforts.
Regards,
On Wed, Jul 24, 2019 at 02:42:24PM +0200, Lynne wrote: > Jul 24, 2019, 11:08 AM by michael@niedermayer.cc: > > > > > What did you expect ? IIRC you have asked for whole classes of security > > issues to be not fixed. > > > > Something like that would require a vote and majority of developers. > > > > The way I interpret this: "Of course I ignored you, you're mental!", doesn't help. And I don't think its just me. You are reading something into this that i have never meant or written > And you do remember incorrectly in saying that I want this whole class of security issues not fixed. In this thread I specifically raised the issue of what is considered to be a security issue by asking whether a speedup of failing to decode from 2 to 0.4 seconds is considered such or what's considered acceptable in general. > And I think I'll disagree that it is. 16 seconds to 2 seconds I can accept, but not 2 to 0.4. These durations are the testcases found by the fuzzer, they say nothing about what the worst case for an issue is. The fuzzer builds a testcase trying to exceed a timeout it stops trying to "improve" it once it found something that takes a few seconds. You can in general make these cases significantly longer running. The reason why the fuzzer doesnt produce hour or day long timeouts is just because it doesnt search for anything longer than a few seconds. > > > > >> These patches affect decoding of real world broken files in favor of fixing specially crafted fuzzed files. > >> > > > > Iam happy to look into such cases. Can you provide me with such > > "real world broken files"? > > Its not intended to worsen the output from such files > > > > Simple logical analysis, "if file is somewhat broken, don't try decoding" does very well indicate that it won't only apply for _this_ broken file, but in general. > Thus, this is for you to prove. I've said it before that otherwise its a burden to other developers to have to screen all of these patches. The changes i do in general, i think about potential effects on slightly broken files and try to test with what iam able to find as matching input material. I find it a bit rude from you that you assume i would not already consider this case. Do i never make a mistake ? well i wish so but iam a human. So again if you know of specific cases where theres a problem, tell me about them please > > > > >> Sure, protecting against ddos attacts is important, but not important enough to make decoders give up early and return nothing. Especially in cases where the timeout speedup is of the order of 2s to 400ms. > >> Yet in all of those timeout patches all you've cared about is shutting up the tool. You've never once shown any figures if this could affect decoding, because its a lot harder than just showing they fix something some tool calls a timeout and forgetting about it. You haven't even commented on this when I asked you on IRC. > >> You also sneak this type of patches in when there's an overflow later on during decoding, which is completely incorrect in almost all cases, for the same reason above. > >> > > > > if you know of issues in a patch or commit you should report this > > during patch review or as soon as you find out about the issue > > as a reply to that patch or commit or as mail to the author. > > > > That's what I'm doing. > That aside, you've completely ignored my statements on what's considered acceptable, showing figures, and sneaking this type of patches to fix undefined behavior. > Making your reply a simple refutation, rather than addressing anything I've said. > So I'm asking you again, what is considered a security issue and what is considered acceptable? And what is considered not a security issue but a complaint from an overzealous automated tool. undefined behavior is unacceptable. Its not allowed in C. It doesnt matter here if its a security issue or not. Timeouts can in general be used for denial of service attacks. While this is less critical than many other security issues it is a security issue. Also for Timeouts many point to bugs, to missing end of input checks, to missing checks in or before loops, to missing EOF checks, to missing checks that the input actually contains enough data resembling a fraction of the smallest half valid frame. We can spend many hours and days arguing if a issue is critical enough to be a security issue. maybe the one we would look at is not but then i still would try to fix it. If we dont fix one it will block the fuzzer from finding another timeout issue in the same decoder. And that one could be security relevant. So fixing as many as possible is the awnser here too About the fuzzer, if it reports a timeout then there is a timeout, if it reports undefined behavior then there is undefined behavior. Always ? no, it contained bugs but that is rather uncommon. Also the fuzzer has no mercy with you, you dont fix some issue, it will be made public and security researchers will look into how to exploit it eventually. If you didnt have the time or manpower, or deadlocked yourself with arguments the fuzzer doesnt care it has a clock and when thats up it publishes all details of an issue. Thanks [...]
On 7/25/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Jul 24, 2019 at 02:42:24PM +0200, Lynne wrote: >> Jul 24, 2019, 11:08 AM by michael@niedermayer.cc: >> >> > >> > What did you expect ? IIRC you have asked for whole classes of security >> > issues to be not fixed. >> > >> > Something like that would require a vote and majority of developers. >> > >> >> The way I interpret this: "Of course I ignored you, you're mental!", >> doesn't help. And I don't think its just me. > > You are reading something into this that i have never meant or written > > >> And you do remember incorrectly in saying that I want this whole class of >> security issues not fixed. In this thread I specifically raised the issue >> of what is considered to be a security issue by asking whether a speedup >> of failing to decode from 2 to 0.4 seconds is considered such or what's >> considered acceptable in general. >> And I think I'll disagree that it is. 16 seconds to 2 seconds I can >> accept, but not 2 to 0.4. > > These durations are the testcases found by the fuzzer, they say nothing > about > what the worst case for an issue is. > The fuzzer builds a testcase trying to exceed a timeout it stops trying to > "improve" it once it found something that takes a few seconds. > > You can in general make these cases significantly longer running. > > The reason why the fuzzer doesnt produce hour or day long timeouts is just > because > it doesnt search for anything longer than a few seconds. > > >> >> >> >> >> These patches affect decoding of real world broken files in favor of >> >> fixing specially crafted fuzzed files. >> >> >> > >> > Iam happy to look into such cases. Can you provide me with such >> > "real world broken files"? >> > Its not intended to worsen the output from such files >> > >> >> Simple logical analysis, "if file is somewhat broken, don't try decoding" >> does very well indicate that it won't only apply for _this_ broken file, >> but in general. >> Thus, this is for you to prove. I've said it before that otherwise its a >> burden to other developers to have to screen all of these patches. > > The changes i do in general, i think about potential effects on > slightly broken files and try to test with what iam able to find as > matching > input material. > I find it a bit rude from you that you assume i would not already consider > this > case. > Do i never make a mistake ? well i wish so but iam a human. So again if you > know of specific cases where theres a problem, tell me about them please > > >> >> >> >> >> Sure, protecting against ddos attacts is important, but not important >> >> enough to make decoders give up early and return nothing. Especially in >> >> cases where the timeout speedup is of the order of 2s to 400ms. >> >> Yet in all of those timeout patches all you've cared about is shutting >> >> up the tool. You've never once shown any figures if this could affect >> >> decoding, because its a lot harder than just showing they fix something >> >> some tool calls a timeout and forgetting about it. You haven't even >> >> commented on this when I asked you on IRC. >> >> You also sneak this type of patches in when there's an overflow later >> >> on during decoding, which is completely incorrect in almost all cases, >> >> for the same reason above. >> >> >> > >> > if you know of issues in a patch or commit you should report this >> > during patch review or as soon as you find out about the issue >> > as a reply to that patch or commit or as mail to the author. >> > >> >> That's what I'm doing. >> That aside, you've completely ignored my statements on what's considered >> acceptable, showing figures, and sneaking this type of patches to fix >> undefined behavior. >> Making your reply a simple refutation, rather than addressing anything >> I've said. >> So I'm asking you again, what is considered a security issue and what is >> considered acceptable? And what is considered not a security issue but a >> complaint from an overzealous automated tool. > > undefined behavior is unacceptable. Its not allowed in C. It doesnt matter > here if its a security issue or not. > > Timeouts can in general be used for denial of service attacks. While this > is less critical than many other security issues it is a security issue. > Also for Timeouts many point to bugs, to missing end of input checks, to > missing > checks in or before loops, to missing EOF checks, to missing checks that > the input actually contains enough data resembling a fraction of the > smallest > half valid frame. > > We can spend many hours and days arguing if a issue is critical enough to > be > a security issue. maybe the one we would look at is not but then i still > would > try to fix it. > If we dont fix one it will block the fuzzer from finding another timeout > issue in the same decoder. And that one could be security relevant. > So fixing as many as possible is the awnser here too > > About the fuzzer, if it reports a timeout then there is a timeout, > if it reports undefined behavior then there is undefined behavior. > Always ? no, it contained bugs but that is rather uncommon. > > Also the fuzzer has no mercy with you, you dont fix some issue, it will be > made public and security researchers will look into how to exploit it > eventually. If you didnt have the time or manpower, or deadlocked > yourself with arguments the fuzzer doesnt care it has a clock and when > thats > up it publishes all details of an issue. Correct fix for fuzzer is to not fuzz incredible big video sizes. > > Thanks > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you think the mosad wants you dead since a long time then you are either > wrong or dead since a long time. >
On 7/25/19, Nicolas George <george@nsup.org> wrote: > Lynne (12019-07-23): >> IRC. > > Each time I ask where something horrible that causes a flamewar between > developers had been said, I get the same answer: IRC. > > I am starting to believe that the problem is IRC. I'm starting to believe its problem in certain developer(s). *hint* *hint*
Jul 25, 2019, 4:47 PM by michael@niedermayer.cc: > On Wed, Jul 24, 2019 at 02:42:24PM +0200, Lynne wrote: > >> Jul 24, 2019, 11:08 AM by michael@niedermayer.cc: >> >> > >> > What did you expect ? IIRC you have asked for whole classes of security >> > issues to be not fixed. >> > >> > Something like that would require a vote and majority of developers. >> > >> >> The way I interpret this: "Of course I ignored you, you're mental!", doesn't help. And I don't think its just me. >> > > You are reading something into this that i have never meant or written > > >> And you do remember incorrectly in saying that I want this whole class of security issues not fixed. In this thread I specifically raised the issue of what is considered to be a security issue by asking whether a speedup of failing to decode from 2 to 0.4 seconds is considered such or what's considered acceptable in general. >> And I think I'll disagree that it is. 16 seconds to 2 seconds I can accept, but not 2 to 0.4. >> > > These durations are the testcases found by the fuzzer, they say nothing about > what the worst case for an issue is. > The fuzzer builds a testcase trying to exceed a timeout it stops trying to > "improve" it once it found something that takes a few seconds. > > You can in general make these cases significantly longer running. > > The reason why the fuzzer doesnt produce hour or day long timeouts is just because > it doesnt search for anything longer than a few seconds. > > >> >> >> >> >> These patches affect decoding of real world broken files in favor of fixing specially crafted fuzzed files. >> >> >> > >> > Iam happy to look into such cases. Can you provide me with such >> > "real world broken files"? >> > Its not intended to worsen the output from such files >> > >> >> Simple logical analysis, "if file is somewhat broken, don't try decoding" does very well indicate that it won't only apply for _this_ broken file, but in general. >> Thus, this is for you to prove. I've said it before that otherwise its a burden to other developers to have to screen all of these patches. >> > > The changes i do in general, i think about potential effects on > slightly broken files and try to test with what iam able to find as matching > input material. > I find it a bit rude from you that you assume i would not already consider this > case. > Do i never make a mistake ? well i wish so but iam a human. So again if you > know of specific cases where theres a problem, tell me about them please > I told you its up to you to find them. I am sorry if I was rude however none of your timeout patches have shown any lenience, regardless of how short of a timeout they addressed. As such, I can't help but feel like you've never thought of what the side effects of the patches were. Just that the fuzzer's random opinion on what a timeout is was more important. >> >> Sure, protecting against ddos attacts is important, but not important enough to make decoders give up early and return nothing. Especially in cases where the timeout speedup is of the order of 2s to 400ms. >> >> Yet in all of those timeout patches all you've cared about is shutting up the tool. You've never once shown any figures if this could affect decoding, because its a lot harder than just showing they fix something some tool calls a timeout and forgetting about it. You haven't even commented on this when I asked you on IRC. >> >> You also sneak this type of patches in when there's an overflow later on during decoding, which is completely incorrect in almost all cases, for the same reason above. >> >> >> > >> > if you know of issues in a patch or commit you should report this >> > during patch review or as soon as you find out about the issue >> > as a reply to that patch or commit or as mail to the author. >> > >> >> That's what I'm doing. >> That aside, you've completely ignored my statements on what's considered acceptable, showing figures, and sneaking this type of patches to fix undefined behavior. >> Making your reply a simple refutation, rather than addressing anything I've said. >> So I'm asking you again, what is considered a security issue and what is considered acceptable? And what is considered not a security issue but a complaint from an overzealous automated tool. >> > > undefined behavior is unacceptable. Its not allowed in C. It doesnt matter > here if its a security issue or not. > > Timeouts can in general be used for denial of service attacks. While this > is less critical than many other security issues it is a security issue. > Also for Timeouts many point to bugs, to missing end of input checks, to missing > checks in or before loops, to missing EOF checks, to missing checks that > the input actually contains enough data resembling a fraction of the smallest > half valid frame. > > We can spend many hours and days arguing if a issue is critical enough to be > a security issue. maybe the one we would look at is not but then i still would > try to fix it. > If we dont fix one it will block the fuzzer from finding another timeout > issue in the same decoder. And that one could be security relevant. > So fixing as many as possible is the awnser here too > > About the fuzzer, if it reports a timeout then there is a timeout, > if it reports undefined behavior then there is undefined behavior. > Always ? no, it contained bugs but that is rather uncommon. > > Also the fuzzer has no mercy with you, you dont fix some issue, it will be > made public and security researchers will look into how to exploit it > eventually. If you didnt have the time or manpower, or deadlocked > yourself with arguments the fuzzer doesnt care it has a clock and when thats > up it publishes all details of an issue. > Stop trying to involve undefined behavior to derail discussion, I asked you a series of questions regarding the timeouts. I disagree with the fuzzer's opinion on what's considered a timeout, and with your fixes of those so-called issues. I don't think that patches which fix timeouts less than 3 seconds or so are worth fixing for the trouble it is to prove they don't affect anything else, unless its plainly obvious. And I don't think timeout fixes should be affected by any options, like the damage percentage one. That's 100% clearly just shutting the fuzzer up, and absolutely nothing else, since that's not something anyone sets ever. Let the fuzzer publish those 3-second "timeouts", big deal. As I've said before, there are much more high profile vulnerabilities that can't be fixed without breaking some files which pretty much everyone knows about, yet no exploits have been made for. As for the fuzzer not finding more timeouts, that's the fuzzer's problem, not ours. It shouldn't have called wolf on all little timeouts it encounters. Or it should filter that "timeout", like Coverity when it makes mistakes. Though unlike Coverity, the fuzzer is a black box that's unfixable, and one more point against it as a tool that shouldn't be trusted. Hence I am asking you to exercise discretion when writing and posting those patches and filter insignificant ones.
On Thu, Jul 25, 2019 at 05:55:02PM +0200, Paul B Mahol wrote: > On 7/25/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, Jul 24, 2019 at 02:42:24PM +0200, Lynne wrote: > >> Jul 24, 2019, 11:08 AM by michael@niedermayer.cc: > >> > >> > > >> > What did you expect ? IIRC you have asked for whole classes of security > >> > issues to be not fixed. > >> > > >> > Something like that would require a vote and majority of developers. > >> > > >> > >> The way I interpret this: "Of course I ignored you, you're mental!", > >> doesn't help. And I don't think its just me. > > > > You are reading something into this that i have never meant or written > > > > > >> And you do remember incorrectly in saying that I want this whole class of > >> security issues not fixed. In this thread I specifically raised the issue > >> of what is considered to be a security issue by asking whether a speedup > >> of failing to decode from 2 to 0.4 seconds is considered such or what's > >> considered acceptable in general. > >> And I think I'll disagree that it is. 16 seconds to 2 seconds I can > >> accept, but not 2 to 0.4. > > > > These durations are the testcases found by the fuzzer, they say nothing > > about > > what the worst case for an issue is. > > The fuzzer builds a testcase trying to exceed a timeout it stops trying to > > "improve" it once it found something that takes a few seconds. > > > > You can in general make these cases significantly longer running. > > > > The reason why the fuzzer doesnt produce hour or day long timeouts is just > > because > > it doesnt search for anything longer than a few seconds. > > > > > >> > >> > >> > >> >> These patches affect decoding of real world broken files in favor of > >> >> fixing specially crafted fuzzed files. > >> >> > >> > > >> > Iam happy to look into such cases. Can you provide me with such > >> > "real world broken files"? > >> > Its not intended to worsen the output from such files > >> > > >> > >> Simple logical analysis, "if file is somewhat broken, don't try decoding" > >> does very well indicate that it won't only apply for _this_ broken file, > >> but in general. > >> Thus, this is for you to prove. I've said it before that otherwise its a > >> burden to other developers to have to screen all of these patches. > > > > The changes i do in general, i think about potential effects on > > slightly broken files and try to test with what iam able to find as > > matching > > input material. > > I find it a bit rude from you that you assume i would not already consider > > this > > case. > > Do i never make a mistake ? well i wish so but iam a human. So again if you > > know of specific cases where theres a problem, tell me about them please > > > > > >> > >> > >> > >> >> Sure, protecting against ddos attacts is important, but not important > >> >> enough to make decoders give up early and return nothing. Especially in > >> >> cases where the timeout speedup is of the order of 2s to 400ms. > >> >> Yet in all of those timeout patches all you've cared about is shutting > >> >> up the tool. You've never once shown any figures if this could affect > >> >> decoding, because its a lot harder than just showing they fix something > >> >> some tool calls a timeout and forgetting about it. You haven't even > >> >> commented on this when I asked you on IRC. > >> >> You also sneak this type of patches in when there's an overflow later > >> >> on during decoding, which is completely incorrect in almost all cases, > >> >> for the same reason above. > >> >> > >> > > >> > if you know of issues in a patch or commit you should report this > >> > during patch review or as soon as you find out about the issue > >> > as a reply to that patch or commit or as mail to the author. > >> > > >> > >> That's what I'm doing. > >> That aside, you've completely ignored my statements on what's considered > >> acceptable, showing figures, and sneaking this type of patches to fix > >> undefined behavior. > >> Making your reply a simple refutation, rather than addressing anything > >> I've said. > >> So I'm asking you again, what is considered a security issue and what is > >> considered acceptable? And what is considered not a security issue but a > >> complaint from an overzealous automated tool. > > > > undefined behavior is unacceptable. Its not allowed in C. It doesnt matter > > here if its a security issue or not. > > > > Timeouts can in general be used for denial of service attacks. While this > > is less critical than many other security issues it is a security issue. > > Also for Timeouts many point to bugs, to missing end of input checks, to > > missing > > checks in or before loops, to missing EOF checks, to missing checks that > > the input actually contains enough data resembling a fraction of the > > smallest > > half valid frame. > > > > We can spend many hours and days arguing if a issue is critical enough to > > be > > a security issue. maybe the one we would look at is not but then i still > > would > > try to fix it. > > If we dont fix one it will block the fuzzer from finding another timeout > > issue in the same decoder. And that one could be security relevant. > > So fixing as many as possible is the awnser here too > > > > About the fuzzer, if it reports a timeout then there is a timeout, > > if it reports undefined behavior then there is undefined behavior. > > Always ? no, it contained bugs but that is rather uncommon. > > > > Also the fuzzer has no mercy with you, you dont fix some issue, it will be > > made public and security researchers will look into how to exploit it > > eventually. If you didnt have the time or manpower, or deadlocked > > yourself with arguments the fuzzer doesnt care it has a clock and when > > thats > > up it publishes all details of an issue. > > > Correct fix for fuzzer is to not fuzz incredible big video sizes. A maximum pixel count of 16M pixels that is 4k by 4k is part of the interface code to the fuzzer since its initial commit into ffmpeg in january 2017 Thanks [...]
Yo, On Tue, Jul 23, 2019, at 04:42, Lynne wrote: > Jul 23, 2019, 12:00 AM by michael@niedermayer.cc: > >> Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different? > > > >> Please, stop it with those patches. It takes energy and some worry to have to open up the ML and scan it for bad patches from people who really should know better every time. > > > > Please stop with these attacks, this is just making people become > > mutually more aggressive and filled with hatred. > > Of course I'm frustrated, I've been ignored. Frustration should not allow this tone on this mailing list. You may have a point on technical matters, but there are better ways to express those, I believe. Because if everyone gets pissed/frustrated, then we won't get anything done. If you know how to fix those things in a proper way, please tell us. I am sure we could even sponsor you for doing this work. Best,
On Mon, Jul 29, 2019 at 10:36:15PM +0200, Jean-Baptiste Kempf wrote: > Yo, > > On Tue, Jul 23, 2019, at 04:42, Lynne wrote: > > Jul 23, 2019, 12:00 AM by michael@niedermayer.cc: > > >> Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different? > > > > > >> Please, stop it with those patches. It takes energy and some worry to have to open up the ML and scan it for bad patches from people who really should know better every time. > > > > > > Please stop with these attacks, this is just making people become > > > mutually more aggressive and filled with hatred. > > > > Of course I'm frustrated, I've been ignored. > > Frustration should not allow this tone on this mailing list. > > You may have a point on technical matters, but there are better ways to express those, I believe. > Because if everyone gets pissed/frustrated, then we won't get anything done. > > If you know how to fix those things in a proper way, please tell us. I am sure we could even sponsor you for doing this work. Iam happy to fix things in a "better way" if a better way is suggested. We should keep in mind though that often different people have different goals and may see different things as "proper" or "better". I think above all we need a bit more understanding that whatever one sees as best is exactly that, ones own preferred solution. And best is not a absolute but a function of many factors. I think with something like that in mind we should be able to find compromises which everyone would be happy with even if it is not everyones "best" choice. And Iam happy to implement these compromises as long as the environment stays friendly. Thanks [...]
On Mon, Jul 22, 2019 at 11:57:24PM +0200, Michael Niedermayer wrote: > The dimensions are always 320x200 they are hardcoded in the demuxer. > Hardcode them instead in the decoder. > > Fixes: Timeout (16sec -> 400ms) > Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> are there any remaining objections to this patch ? thanks [...]
diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c index 6662979c52..2d336a61e5 100644 --- a/libavcodec/rl2.c +++ b/libavcodec/rl2.c @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext *avctx) Rl2Context *s = avctx->priv_data; int back_size; int i; + int ret; s->avctx = avctx; avctx->pix_fmt = AV_PIX_FMT_PAL8; + ret = ff_set_dimensions(avctx, 320, 200); + if (ret < 0) + return ret; + /** parse extra data */ if (!avctx->extradata || avctx->extradata_size < EXTRADATA1_SIZE) { av_log(avctx, AV_LOG_ERROR, "invalid extradata size\n");
The dimensions are always 320x200 they are hardcoded in the demuxer. Hardcode them instead in the decoder. Fixes: Timeout (16sec -> 400ms) Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/rl2.c | 5 +++++ 1 file changed, 5 insertions(+)