| Message ID | 8842f859-7257-dabd-bc86-14aba952e547@googlemail.com |
|---|---|
| State | Accepted |
| Commit | 9ec8790ac4787d3d514c5fa27b66d581614fd1be |
| Headers | show |
On Thu, Jan 26, 2017 at 02:14:09AM +0100, Andreas Cadhalpun wrote: > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavformat/boadec.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) LGTM thx [...]
On 27.01.2017 03:27, Michael Niedermayer wrote: > On Thu, Jan 26, 2017 at 02:14:09AM +0100, Andreas Cadhalpun wrote: >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> --- >> libavformat/boadec.c | 14 +++++++++++++- >> 1 file changed, 13 insertions(+), 1 deletion(-) > > LGTM Pushed. Best regards, Andreas
Hi, On Sat, Jan 28, 2017 at 7:25 PM, Andreas Cadhalpun < andreas.cadhalpun@googlemail.com> wrote: > On 27.01.2017 03:27, Michael Niedermayer wrote: > > On Thu, Jan 26, 2017 at 02:14:09AM +0100, Andreas Cadhalpun wrote: > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > >> --- > >> libavformat/boadec.c | 14 +++++++++++++- > >> 1 file changed, 13 insertions(+), 1 deletion(-) > > > > LGTM > > Pushed. Hm ... So I guess I wasn't clear about this, but the reason I didn't reply to other patches with log messages was not because I'm OK with, but simply to keep the discussion contained in a single thread and not spam the list. I'd prefer if the log msg disappeared from all fuzz-only checks... Ronald
On 29.01.2017 04:46, Ronald S. Bultje wrote: > Hm ... So I guess I wasn't clear about this, but the reason I didn't reply > to other patches with log messages was not because I'm OK with, but simply > to keep the discussion contained in a single thread and not spam the list. > I'd prefer if the log msg disappeared from all fuzz-only checks... Being a "fuzz-only check" is not a well-defined concept. Anything a fuzzer does could in principle also happen due to file corruption. For header parsing such errors could also happen if a file gets misdetected and thus a wrong demuxer is used. So what do you mean with "fuzz-only check"? For example, would you consider the error check I quoted in the other thread [1] a "fuzz-only check"? It's clear that you prefer fewer log messages than I do, but in the absence of a general consensus about this topic, every author/maintainer can decide which log messages are wanted in their own code. Best regards, Andreas 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206312.html
Hi, On Sun, Jan 29, 2017 at 7:37 PM, Andreas Cadhalpun < andreas.cadhalpun@googlemail.com> wrote: > On 29.01.2017 04:46, Ronald S. Bultje wrote: > > Hm ... So I guess I wasn't clear about this, but the reason I didn't > reply > > to other patches with log messages was not because I'm OK with, but > simply > > to keep the discussion contained in a single thread and not spam the > list. > > I'd prefer if the log msg disappeared from all fuzz-only checks... > > Being a "fuzz-only check" is not a well-defined concept. Anything a fuzzer > does could in principle also happen due to file corruption. Please stop already. This is ridiculous. Error messages are supposed to help solve a problem. A corrupt file isn't solvable. No error message can help. So why bother informing the user? "Channel count too large" is the same as "fluffybuzz whackabit mackahooloo". There is no action associated with the message that can help solve it. This is different from "encryption key missing, please use the option -key value to specify" for encrypted content. All we're doing is growing source and binary size and code complexity, and we do so without any apparent benefit. I just don't think that's a great approach. Ronald
On 30.01.2017 17:17, Ronald S. Bultje wrote: > On Sun, Jan 29, 2017 at 7:37 PM, Andreas Cadhalpun < > andreas.cadhalpun@googlemail.com> wrote: > >> On 29.01.2017 04:46, Ronald S. Bultje wrote: >>> Hm ... So I guess I wasn't clear about this, but the reason I didn't >> reply >>> to other patches with log messages was not because I'm OK with, but >> simply >>> to keep the discussion contained in a single thread and not spam the >> list. >>> I'd prefer if the log msg disappeared from all fuzz-only checks... >> >> Being a "fuzz-only check" is not a well-defined concept. Anything a fuzzer >> does could in principle also happen due to file corruption. > > > Please stop already. This is ridiculous. Please don't ignore the rest of my argument nor laugh at it. > Error messages are supposed to help solve a problem. A corrupt file isn't > solvable. It can be solved by restoring a backup, for example. > No error message can help. So why bother informing the user? To give the user some clue what's wrong. It's impossible to tell in advance, whether or not that will actually help him. > "Channel count too large" is the same as "fluffybuzz whackabit > mackahooloo". It obviously isn't: Unlike the latter, the former can be understood by most English speaking persons. Also the log message added here was "Too many channels", which already had 9 occurrences in the code base and which is quite similar to "Too many invisible frames", which you added. It is completely arbitrary that you bikeshed my patches, while you apparently have no problem with those. > There is no action associated with the message that can help > solve it. This is different from "encryption key missing, please use the > option -key value to specify" for encrypted content. Almost no log message in the ffmpeg code base contains a direct action. Do you want to remove all other log messages? > All we're doing is growing source and binary size and code complexity, and > we do so without any apparent benefit. I just don't think that's a great > approach. Marton proposed a way to mitigate this [1], but you haven't commented on it so far. Best regards, Andreas 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206327.html
Hi, On Mon, Jan 30, 2017 at 8:52 PM, Andreas Cadhalpun < andreas.cadhalpun@googlemail.com> wrote: > Marton proposed a way to mitigate this [1], but you haven't commented > on it so far. It doesn't mitigate it. Source code is still an issue, as is binary size if CONFIG_SMALL is off, which is the default for release builds. Even if you change CONFIG_SMALL to NDEBUG, the source code issue is still there. I don't want this patch. I also don't want to discuss it further. Please remove the log message. Thank you. Ronald
On Tue, Jan 31, 2017 at 07:45:56AM -0500, Ronald S. Bultje wrote: > Hi, > > On Mon, Jan 30, 2017 at 8:52 PM, Andreas Cadhalpun < > andreas.cadhalpun@googlemail.com> wrote: > > > Marton proposed a way to mitigate this [1], but you haven't commented > > on it so far. > > > It doesn't mitigate it. Source code is still an issue, as is binary size if > CONFIG_SMALL is off, which is the default for release builds. > > Even if you change CONFIG_SMALL to NDEBUG, the source code issue is still > there. > > I don't want this patch. I also don't want to discuss it further. Please > remove the log message. Thank you. > I stayed silent on the issue so far, but I'm with Ronald on this one and don't want to pollute functional code with elaborated error code paths unlikely to happen. I understand every con and pro from both sides and don't plan to comment again on the topic. Regards,
On 31.01.2017 13:45, Ronald S. Bultje wrote: > I don't want this patch. I also don't want to discuss it further. Please > remove the log message. Thank you. For the sake of ending this, I've removed them. Please avoid complaining about log messages for my future patches. Thank you. Best regards, Andreas
diff --git a/libavformat/boadec.c b/libavformat/boadec.c index ac2a33b3f0..6055effcad 100644 --- a/libavformat/boadec.c +++ b/libavformat/boadec.c @@ -20,6 +20,7 @@ */ #include "libavutil/intreadwrite.h" +#include "libavcodec/internal.h" #include "avformat.h" #include "internal.h" @@ -53,9 +54,20 @@ static int read_header(AVFormatContext *s) avio_rl32(s->pb); st->codecpar->sample_rate = avio_rl32(s->pb); st->codecpar->channels = avio_rl32(s->pb); + if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { + av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n", + st->codecpar->channels, FF_SANE_NB_CHANNELS); + return AVERROR(ENOSYS); + } s->internal->data_offset = avio_rl32(s->pb); avio_r8(s->pb); - st->codecpar->block_align = st->codecpar->channels * avio_rl32(s->pb); + st->codecpar->block_align = avio_rl32(s->pb); + if (st->codecpar->block_align > INT_MAX / FF_SANE_NB_CHANNELS) { + av_log(s, AV_LOG_ERROR, "Too large block alignment %d > %d\n", + st->codecpar->block_align, INT_MAX / FF_SANE_NB_CHANNELS); + return AVERROR_INVALIDDATA; + } + st->codecpar->block_align *= st->codecpar->channels; avio_seek(s->pb, s->internal->data_offset, SEEK_SET);
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavformat/boadec.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)