| Message ID | d295ffde-47d7-4cb2-4732-625336fa43da@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < andreas.cadhalpun@googlemail.com> wrote: > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavformat/nistspheredec.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c > index 782d1dfbfb..3386497682 100644 > --- a/libavformat/nistspheredec.c > +++ b/libavformat/nistspheredec.c > @@ -21,6 +21,7 @@ > > #include "libavutil/avstring.h" > #include "libavutil/intreadwrite.h" > +#include "libavcodec/internal.h" > #include "avformat.h" > #include "internal.h" > #include "pcm.h" > @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) > return 0; > } else if (!memcmp(buffer, "channel_count", 13)) { > sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); > + 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); > + } I've said this before, but again - please don't add useless log messages. These don't help end users at all, since these samples are exceedingly unlikely to be real. Most likely, they derive from fuzzing, and stderr cramming is one of the things that makes fuzzing slower. Ronald
On 26.01.2017 02:29, Ronald S. Bultje wrote: > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < > andreas.cadhalpun@googlemail.com> wrote: > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> --- >> libavformat/nistspheredec.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c >> index 782d1dfbfb..3386497682 100644 >> --- a/libavformat/nistspheredec.c >> +++ b/libavformat/nistspheredec.c >> @@ -21,6 +21,7 @@ >> >> #include "libavutil/avstring.h" >> #include "libavutil/intreadwrite.h" >> +#include "libavcodec/internal.h" >> #include "avformat.h" >> #include "internal.h" >> #include "pcm.h" >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) >> return 0; >> } else if (!memcmp(buffer, "channel_count", 13)) { >> sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); >> + 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); >> + } > > > I've said this before, but again - please don't add useless log messages. I disagree that these log messages are useless and I'm not the only one [1]. > These don't help end users at all, since these samples are exceedingly > unlikely to be real. Files can get corrupted randomly, so I think this error isn't less likely than most other errors. > Most likely, they derive from fuzzing, and stderr > cramming is one of the things that makes fuzzing slower. Printing log messages in inner decoding loops makes that slower, but a log message during header parsing is hardly noticeable. Best regards, Andreas 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205433.html
On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: > On 26.01.2017 02:29, Ronald S. Bultje wrote: > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < > > andreas.cadhalpun@googlemail.com> wrote: > > > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > >> --- > >> libavformat/nistspheredec.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c > >> index 782d1dfbfb..3386497682 100644 > >> --- a/libavformat/nistspheredec.c > >> +++ b/libavformat/nistspheredec.c > >> @@ -21,6 +21,7 @@ > >> > >> #include "libavutil/avstring.h" > >> #include "libavutil/intreadwrite.h" > >> +#include "libavcodec/internal.h" > >> #include "avformat.h" > >> #include "internal.h" > >> #include "pcm.h" > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) > >> return 0; > >> } else if (!memcmp(buffer, "channel_count", 13)) { > >> sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); > >> + 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); > >> + } > > > > > > I've said this before, but again - please don't add useless log messages. > > I disagree that these log messages are useless and I'm not the only one [1]. +1 Log messages make debuging the code easier, as a developer i like to know why something fails having a hard failure but no clear and easy findable error message is bad [...]
On Thu, 26 Jan 2017, Michael Niedermayer wrote: > On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: >> On 26.01.2017 02:29, Ronald S. Bultje wrote: >>> On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < >>> andreas.cadhalpun@googlemail.com> wrote: >>> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >>>> --- >>>> libavformat/nistspheredec.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c >>>> index 782d1dfbfb..3386497682 100644 >>>> --- a/libavformat/nistspheredec.c >>>> +++ b/libavformat/nistspheredec.c >>>> @@ -21,6 +21,7 @@ >>>> >>>> #include "libavutil/avstring.h" >>>> #include "libavutil/intreadwrite.h" >>>> +#include "libavcodec/internal.h" >>>> #include "avformat.h" >>>> #include "internal.h" >>>> #include "pcm.h" >>>> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) >>>> return 0; >>>> } else if (!memcmp(buffer, "channel_count", 13)) { >>>> sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); >>>> + 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); >>>> + } >>> >>> >>> I've said this before, but again - please don't add useless log messages. >> >> I disagree that these log messages are useless and I'm not the only one [1]. > > +1 > > Log messages make debuging the code easier, as a developer i like to > know why something fails having a hard failure but no clear and easy > findable error message is bad > I tend to agree with Ronald here, log messages which are practically impossible to trigger with real-world files have little benefit, also I don't think it is ffmpeg's job to thoroughly explain every different kind of error. Maybe some middle ground can be found defining macros so the user can decide if he wants these messages in the build or not, also maybe some "probability" levels can be added to these errors so the macros can work based on them, and finally a lot of similar checks exist around the codebase which could be factorized, so the code - even with reported messages - would look less cluttered with functionally useless parts. Regards, Marton
On Thu, Jan 26, 2017 at 03:52:04AM +0100, Marton Balint wrote: > > On Thu, 26 Jan 2017, Michael Niedermayer wrote: > > >On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: > >>On 26.01.2017 02:29, Ronald S. Bultje wrote: > >>>On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < > >>>andreas.cadhalpun@googlemail.com> wrote: > >>> > >>>>Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > >>>>--- > >>>> libavformat/nistspheredec.c | 11 +++++++++++ > >>>> 1 file changed, 11 insertions(+) > >>>> > >>>>diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c > >>>>index 782d1dfbfb..3386497682 100644 > >>>>--- a/libavformat/nistspheredec.c > >>>>+++ b/libavformat/nistspheredec.c > >>>>@@ -21,6 +21,7 @@ > >>>> > >>>> #include "libavutil/avstring.h" > >>>> #include "libavutil/intreadwrite.h" > >>>>+#include "libavcodec/internal.h" > >>>> #include "avformat.h" > >>>> #include "internal.h" > >>>> #include "pcm.h" > >>>>@@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) > >>>> return 0; > >>>> } else if (!memcmp(buffer, "channel_count", 13)) { > >>>> sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); > >>>>+ 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); > >>>>+ } > >>> > >>> > >>>I've said this before, but again - please don't add useless log messages. > >> > >>I disagree that these log messages are useless and I'm not the only one [1]. > > > >+1 > > > >Log messages make debuging the code easier, as a developer i like to > >know why something fails having a hard failure but no clear and easy > >findable error message is bad > > > > I tend to agree with Ronald here, log messages which are practically > impossible to trigger with real-world files have little benefit, > also I don't think it is ffmpeg's job to thoroughly explain every > different kind of error. would you want a log message be removed if the people working on the code want the error message to be there ? You seem to just say that you see little benefit in it. [...]
On Thu, 26 Jan 2017 03:20:02 +0100 Michael Niedermayer <michaelni@gmx.at> wrote: > On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: > > On 26.01.2017 02:29, Ronald S. Bultje wrote: > > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < > > > andreas.cadhalpun@googlemail.com> wrote: > > > > > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > > >> --- > > >> libavformat/nistspheredec.c | 11 +++++++++++ > > >> 1 file changed, 11 insertions(+) > > >> > > >> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c > > >> index 782d1dfbfb..3386497682 100644 > > >> --- a/libavformat/nistspheredec.c > > >> +++ b/libavformat/nistspheredec.c > > >> @@ -21,6 +21,7 @@ > > >> > > >> #include "libavutil/avstring.h" > > >> #include "libavutil/intreadwrite.h" > > >> +#include "libavcodec/internal.h" > > >> #include "avformat.h" > > >> #include "internal.h" > > >> #include "pcm.h" > > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) > > >> return 0; > > >> } else if (!memcmp(buffer, "channel_count", 13)) { > > >> sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); > > >> + 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); > > >> + } > > > > > > > > > I've said this before, but again - please don't add useless log messages. > > > > I disagree that these log messages are useless and I'm not the only one [1]. > > +1 > > Log messages make debuging the code easier, as a developer i like to > know why something fails having a hard failure but no clear and easy > findable error message is bad > > > [...] -1 This kind of things bloat the code with rare corner cases. One point would be that this increases binary size (why do we even still have the NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but the current use of the macro will barely even make a dent into the number of bytes "wasted" for optional strings, yet we bother). Another point is that code becomes unreadable if obscure corner cases take up most of the code. I think that's w worrisome direction we're taking here. When I debug FFmpeg API use, the thing that annoys me most is that I can't know where an error happens. I have to trace the origin manually. Error codes are almost always completely useless. This patchset adds a lot of NOSYS, which is used 254 times in FFmpeg and which is about 99% meaningless and resolves to an even worse error message when using av_err2str(). Adding an av_log to error error point would "work" (at least you could use grep), but isn't a good solution. In this particular case, I'd question why the code calling this function (avformat_open_inputavcodec_open) doesn't print an error message instead, or so. But of course not every API user wants such an error message (but still wants others).
On 1/26/17, wm4 <nfxjfg@googlemail.com> wrote: > On Thu, 26 Jan 2017 03:20:02 +0100 > Michael Niedermayer <michaelni@gmx.at> wrote: > >> On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: >> > On 26.01.2017 02:29, Ronald S. Bultje wrote: >> > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < >> > > andreas.cadhalpun@googlemail.com> wrote: >> > > >> > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> > >> --- >> > >> libavformat/nistspheredec.c | 11 +++++++++++ >> > >> 1 file changed, 11 insertions(+) >> > >> >> > >> diff --git a/libavformat/nistspheredec.c >> > >> b/libavformat/nistspheredec.c >> > >> index 782d1dfbfb..3386497682 100644 >> > >> --- a/libavformat/nistspheredec.c >> > >> +++ b/libavformat/nistspheredec.c >> > >> @@ -21,6 +21,7 @@ >> > >> >> > >> #include "libavutil/avstring.h" >> > >> #include "libavutil/intreadwrite.h" >> > >> +#include "libavcodec/internal.h" >> > >> #include "avformat.h" >> > >> #include "internal.h" >> > >> #include "pcm.h" >> > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) >> > >> return 0; >> > >> } else if (!memcmp(buffer, "channel_count", 13)) { >> > >> sscanf(buffer, "%*s %*s %"SCNd32, >> > >> &st->codecpar->channels); >> > >> + 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); >> > >> + } >> > > >> > > >> > > I've said this before, but again - please don't add useless log >> > > messages. >> > >> > I disagree that these log messages are useless and I'm not the only one >> > [1]. >> >> +1 >> >> Log messages make debuging the code easier, as a developer i like to >> know why something fails having a hard failure but no clear and easy >> findable error message is bad >> >> >> [...] > > -1 > > This kind of things bloat the code with rare corner cases. One point > would be that this increases binary size (why do we even still have the > NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but > the current use of the macro will barely even make a dent into the > number of bytes "wasted" for optional strings, yet we bother). > > Another point is that code becomes unreadable if obscure corner cases > take up most of the code. I think that's w worrisome direction we're > taking here. > > When I debug FFmpeg API use, the thing that annoys me most is that I > can't know where an error happens. I have to trace the origin manually. > Error codes are almost always completely useless. This patchset adds a > lot of NOSYS, which is used 254 times in FFmpeg and which is about 99% > meaningless and resolves to an even worse error message when using > av_err2str(). Adding an av_log to error error point would "work" (at > least you could use grep), but isn't a good solution. > > In this particular case, I'd question why the code calling this > function (avformat_open_inputavcodec_open) doesn't print an error > message instead, or so. > > But of course not every API user wants such an error message (but still > wants others). I don't want those log messages in there.
On Thu, Jan 26, 2017 at 06:43:45AM +0100, wm4 wrote: > On Thu, 26 Jan 2017 03:20:02 +0100 > Michael Niedermayer <michaelni@gmx.at> wrote: > > > On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: > > > On 26.01.2017 02:29, Ronald S. Bultje wrote: > > > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < > > > > andreas.cadhalpun@googlemail.com> wrote: > > > > > > > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > > > >> --- > > > >> libavformat/nistspheredec.c | 11 +++++++++++ > > > >> 1 file changed, 11 insertions(+) > > > >> > > > >> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c > > > >> index 782d1dfbfb..3386497682 100644 > > > >> --- a/libavformat/nistspheredec.c > > > >> +++ b/libavformat/nistspheredec.c > > > >> @@ -21,6 +21,7 @@ > > > >> > > > >> #include "libavutil/avstring.h" > > > >> #include "libavutil/intreadwrite.h" > > > >> +#include "libavcodec/internal.h" > > > >> #include "avformat.h" > > > >> #include "internal.h" > > > >> #include "pcm.h" > > > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) > > > >> return 0; > > > >> } else if (!memcmp(buffer, "channel_count", 13)) { > > > >> sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); > > > >> + 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); > > > >> + } > > > > > > > > > > > > I've said this before, but again - please don't add useless log messages. > > > > > > I disagree that these log messages are useless and I'm not the only one [1]. > > > > +1 > > > > Log messages make debuging the code easier, as a developer i like to > > know why something fails having a hard failure but no clear and easy > > findable error message is bad > > > > > > [...] > > -1 > > This kind of things bloat the code with rare corner cases. One point > would be that this increases binary size (why do we even still have the > NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but > the current use of the macro will barely even make a dent into the > number of bytes "wasted" for optional strings, yet we bother). > > Another point is that code becomes unreadable if obscure corner cases > take up most of the code. I think that's w worrisome direction we're > taking here. While it doesnt apply to this patch here but, Error messages in obscure checks that describe the error condition in the source would make at least some checks easier to understand than just a generic "return AVERROR_INVALIDDATA" > > When I debug FFmpeg API use, the thing that annoys me most is that I > can't know where an error happens. I have to trace the origin manually. > Error codes are almost always completely useless. This patchset adds a > lot of NOSYS, which is used 254 times in FFmpeg and which is about 99% > meaningless and resolves to an even worse error message when using > av_err2str(). Adding an av_log to error error point would "work" (at > least you could use grep), but isn't a good solution. I think the idea about something more informative than a int32 error code did come up previously. I certainly would be in favor of having an error value that could be used to pinpoint the error location(s) and or function(s) it passed through, this would be usefull for debguging in general [...]
On Thu, 26 Jan 2017, Michael Niedermayer wrote: > On Thu, Jan 26, 2017 at 03:52:04AM +0100, Marton Balint wrote: >> >> On Thu, 26 Jan 2017, Michael Niedermayer wrote: >> >>> On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: >>>> On 26.01.2017 02:29, Ronald S. Bultje wrote: >>>>> On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < >>>>> andreas.cadhalpun@googlemail.com> wrote: >>>>> >>>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >>>>>> --- >>>>>> libavformat/nistspheredec.c | 11 +++++++++++ >>>>>> 1 file changed, 11 insertions(+) >>>>>> >>>>>> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c >>>>>> index 782d1dfbfb..3386497682 100644 >>>>>> --- a/libavformat/nistspheredec.c >>>>>> +++ b/libavformat/nistspheredec.c >>>>>> @@ -21,6 +21,7 @@ >>>>>> >>>>>> #include "libavutil/avstring.h" >>>>>> #include "libavutil/intreadwrite.h" >>>>>> +#include "libavcodec/internal.h" >>>>>> #include "avformat.h" >>>>>> #include "internal.h" >>>>>> #include "pcm.h" >>>>>> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) >>>>>> return 0; >>>>>> } else if (!memcmp(buffer, "channel_count", 13)) { >>>>>> sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); >>>>>> + 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); >>>>>> + } >>>>> >>>>> >>>>> I've said this before, but again - please don't add useless log messages. >>>> >>>> I disagree that these log messages are useless and I'm not the only one [1]. >>> >>> +1 >>> >>> Log messages make debuging the code easier, as a developer i like to >>> know why something fails having a hard failure but no clear and easy >>> findable error message is bad >>> >> >> I tend to agree with Ronald here, log messages which are practically >> impossible to trigger with real-world files have little benefit, >> also I don't think it is ffmpeg's job to thoroughly explain every >> different kind of error. > > would you want a log message be removed if the > people working on the code want the error message to be there ? > You seem to just say that you see little benefit in it. > Yeah, because as far as I understand the reason behind this, it is only there to let the fuzzer people know why a fuzzed file fails. If we don't draw the line somewhere, ffmpeg will be an analyzer and not a decoder. I see 3 problems (wm4 explicitly named them, but I also had them in mind) - Little benefit, yet - Makes the code less clean, more cluttered - Increases binary size The ideas I proposed (use macros, use common / factorized checks for common validatons and errors) might be a good compromise IMHO. Fuzzing thereforce can be done with "debug" builds. Anyway, I am not blocking the patch, just expressing what I would prefer in the long run. Regards, Marton
On Thu, 26 Jan 2017 17:02:37 +0100 Michael Niedermayer <michaelni@gmx.at> wrote: > On Thu, Jan 26, 2017 at 06:43:45AM +0100, wm4 wrote: > > On Thu, 26 Jan 2017 03:20:02 +0100 > > Michael Niedermayer <michaelni@gmx.at> wrote: > > > > > On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: > > > > On 26.01.2017 02:29, Ronald S. Bultje wrote: > > > > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < > > > > > andreas.cadhalpun@googlemail.com> wrote: > > > > > > > > > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > > > > >> --- > > > > >> libavformat/nistspheredec.c | 11 +++++++++++ > > > > >> 1 file changed, 11 insertions(+) > > > > >> > > > > >> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c > > > > >> index 782d1dfbfb..3386497682 100644 > > > > >> --- a/libavformat/nistspheredec.c > > > > >> +++ b/libavformat/nistspheredec.c > > > > >> @@ -21,6 +21,7 @@ > > > > >> > > > > >> #include "libavutil/avstring.h" > > > > >> #include "libavutil/intreadwrite.h" > > > > >> +#include "libavcodec/internal.h" > > > > >> #include "avformat.h" > > > > >> #include "internal.h" > > > > >> #include "pcm.h" > > > > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) > > > > >> return 0; > > > > >> } else if (!memcmp(buffer, "channel_count", 13)) { > > > > >> sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); > > > > >> + 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); > > > > >> + } > > > > > > > > > > > > > > > I've said this before, but again - please don't add useless log messages. > > > > > > > > I disagree that these log messages are useless and I'm not the only one [1]. > > > > > > +1 > > > > > > Log messages make debuging the code easier, as a developer i like to > > > know why something fails having a hard failure but no clear and easy > > > findable error message is bad > > > > > > > > > [...] > > > > -1 > > > > This kind of things bloat the code with rare corner cases. One point > > would be that this increases binary size (why do we even still have the > > NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but > > the current use of the macro will barely even make a dent into the > > number of bytes "wasted" for optional strings, yet we bother). > > > > Another point is that code becomes unreadable if obscure corner cases > > take up most of the code. I think that's w worrisome direction we're > > taking here. > > While it doesnt apply to this patch here but, > Error messages in obscure checks that describe the error condition > in the source would make at least some checks easier to understand > than just a generic > "return AVERROR_INVALIDDATA" In my opinion, there's no choice but to debug such cases manually anyway. Whether you think that log messages help you better than breakpoints in a source debugger is a different question. > > > > When I debug FFmpeg API use, the thing that annoys me most is that I > > can't know where an error happens. I have to trace the origin manually. > > Error codes are almost always completely useless. This patchset adds a > > lot of NOSYS, which is used 254 times in FFmpeg and which is about 99% > > meaningless and resolves to an even worse error message when using > > av_err2str(). Adding an av_log to error error point would "work" (at > > least you could use grep), but isn't a good solution. > > I think the idea about something more informative than a int32 error > code did come up previously. > I certainly would be in favor of having an error value that could be > used to pinpoint the error location(s) and or function(s) it passed > through, this would be usefull for debguging in general We could probably return a struct that contains the __FILE__ etc. value of the error site, but this would probably be too over-engineered and intrusive for a real solution. Back to this patch and the core issue: I think there's no ideal solution. So balance is required. Don't add av_logs to every error, but add them to cases that are interesting and would be otherwise hard to find. I think fine grained error logging for read_header in a really obscure and tiny demuxer isn't an interesting/useful case.
On 27.01.2017 02:56, Marton Balint wrote: > I see 3 problems (wm4 explicitly named them, but I also had them in mind) > - Little benefit, yet > - Makes the code less clean, more cluttered > - Increases binary size > > The ideas I proposed (use macros, use common / factorized checks for common > validatons and errors) might be a good compromise IMHO. Fuzzing thereforce > can be done with "debug" builds. > > Anyway, I am not blocking the patch, just expressing what I would prefer in > the long run. How about the following macro? #define FF_RETURN_ERROR(condition, error, log_ctx, ...) { \ if (condition) { \ if (!CONFIG_SMALL && log_ctx) \ av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \ return error; \ } \ } That could be used with error message: FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, AVERROR(ENOSYS), s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS) Or without: FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, AVERROR(ENOSYS), NULL) Best regards, Andreas
On Sat, Jan 28, 2017 at 01:26:40AM +0100, Andreas Cadhalpun wrote: > On 27.01.2017 02:56, Marton Balint wrote: > > I see 3 problems (wm4 explicitly named them, but I also had them in mind) > > - Little benefit, yet > > - Makes the code less clean, more cluttered > > - Increases binary size > > > > The ideas I proposed (use macros, use common / factorized checks for common > > validatons and errors) might be a good compromise IMHO. Fuzzing thereforce > > can be done with "debug" builds. > > > > Anyway, I am not blocking the patch, just expressing what I would prefer in > > the long run. > > How about the following macro? > #define FF_RETURN_ERROR(condition, error, log_ctx, ...) { \ > if (condition) { \ > if (!CONFIG_SMALL && log_ctx) \ > av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \ > return error; \ > } \ > } > > That could be used with error message: > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, AVERROR(ENOSYS), > s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS) > > Or without: > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, AVERROR(ENOSYS), NULL) I suggest if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog(s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); } or for the 2nd example: if (st->codecpar->channels > FF_SANE_NB_CHANNELS) return AVERROR(ENOSYS); ff_elog() can then be defined to nothing based on CONFIG_SMALL the difference between my suggestion and yours is that mine has new-lines seperating the condition, message and return and that its self explanatory C code. What you do is removing newlines and standard C keywords with a custom macro that people not working on FFmpeg on a regular basis will have difficulty understanding The macro is similar to writing (minus the C keywords) if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog( s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); } we dont do that becuause its just bad code why does this suddenly become desirable when its in a macro? Or maybe the question should be, does code become less cluttered and cleaner when newlines and C keywords are removed ? if not why is that done here ? if yes why is it done just here ? [...]
Hi, On Fri, Jan 27, 2017 at 9:28 PM, Michael Niedermayer <michaelni@gmx.at> wrote: > On Sat, Jan 28, 2017 at 01:26:40AM +0100, Andreas Cadhalpun wrote: > > On 27.01.2017 02:56, Marton Balint wrote: > > > I see 3 problems (wm4 explicitly named them, but I also had them in > mind) > > > - Little benefit, yet > > > - Makes the code less clean, more cluttered > > > - Increases binary size > > > > > > The ideas I proposed (use macros, use common / factorized checks for > common > > > validatons and errors) might be a good compromise IMHO. Fuzzing > thereforce > > > can be done with "debug" builds. > > > > > > Anyway, I am not blocking the patch, just expressing what I would > prefer in > > > the long run. > > > > How about the following macro? > > #define FF_RETURN_ERROR(condition, error, log_ctx, ...) { \ > > if (condition) { \ > > if (!CONFIG_SMALL && log_ctx) \ > > av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \ > > return error; \ > > } \ > > } > > > > That could be used with error message: > > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, > AVERROR(ENOSYS), > > s, "Too many channels %d > %d\n", > st->codecpar->channels, FF_SANE_NB_CHANNELS) > > > > Or without: > > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, > AVERROR(ENOSYS), NULL) > > I suggest > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { > ff_elog(s, "Too many channels %d > %d\n", st->codecpar->channels, > FF_SANE_NB_CHANNELS); > return AVERROR(ENOSYS); > } > > or for the 2nd example: > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) > return AVERROR(ENOSYS); > > > ff_elog() can then be defined to nothing based on CONFIG_SMALL > > the difference between my suggestion and yours is that mine has > new-lines seperating the condition, message and return and that its > self explanatory C code. > > What you do is removing newlines and standard C keywords with a custom > macro that people not working on FFmpeg on a regular basis will have > difficulty understanding > > The macro is similar to writing (minus the C keywords) > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog( > s, "Too many channels %d > %d\n", st->codecpar->channels, > FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); } > > we dont do that becuause its just bad code > why does this suddenly become desirable when its in a macro? > > Or maybe the question should be, does code become less cluttered and > cleaner when newlines and C keywords are removed ? > if not why is that done here ? > if yes why is it done just here ? I agree a macro here doesn't help. My concern wasn't with the check itself, I agree a file with 100 channels should error out. My concern is that these files will universally be the result of fuzzing, so I don't want to spam stderr with messages related to it, nor do I want source/binary size to increase because of it. If you make ff_elog similar to assert (only if NDEBUG is not set), that may work for the binary size concern, but the source code size is still a concern. Again, not because it's bad code, but because it's needless since it only happens for fuzzed samples. Ronald
On Fri, 27 Jan 2017 21:48:05 -0500 "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > Hi, > > On Fri, Jan 27, 2017 at 9:28 PM, Michael Niedermayer <michaelni@gmx.at> > wrote: > > > On Sat, Jan 28, 2017 at 01:26:40AM +0100, Andreas Cadhalpun wrote: > > > On 27.01.2017 02:56, Marton Balint wrote: > > > > I see 3 problems (wm4 explicitly named them, but I also had them in > > mind) > > > > - Little benefit, yet > > > > - Makes the code less clean, more cluttered > > > > - Increases binary size > > > > > > > > The ideas I proposed (use macros, use common / factorized checks for > > common > > > > validatons and errors) might be a good compromise IMHO. Fuzzing > > thereforce > > > > can be done with "debug" builds. > > > > > > > > Anyway, I am not blocking the patch, just expressing what I would > > prefer in > > > > the long run. > > > > > > How about the following macro? > > > #define FF_RETURN_ERROR(condition, error, log_ctx, ...) { \ > > > if (condition) { \ > > > if (!CONFIG_SMALL && log_ctx) \ > > > av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \ > > > return error; \ > > > } \ > > > } > > > > > > That could be used with error message: > > > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, > > AVERROR(ENOSYS), > > > s, "Too many channels %d > %d\n", > > st->codecpar->channels, FF_SANE_NB_CHANNELS) > > > > > > Or without: > > > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, > > AVERROR(ENOSYS), NULL) > > > > I suggest > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { > > ff_elog(s, "Too many channels %d > %d\n", st->codecpar->channels, > > FF_SANE_NB_CHANNELS); > > return AVERROR(ENOSYS); > > } > > > > or for the 2nd example: > > > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) > > return AVERROR(ENOSYS); > > > > > > ff_elog() can then be defined to nothing based on CONFIG_SMALL > > > > the difference between my suggestion and yours is that mine has > > new-lines seperating the condition, message and return and that its > > self explanatory C code. > > > > What you do is removing newlines and standard C keywords with a custom > > macro that people not working on FFmpeg on a regular basis will have > > difficulty understanding > > > > The macro is similar to writing (minus the C keywords) > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog( > > s, "Too many channels %d > %d\n", st->codecpar->channels, > > FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); } > > > > we dont do that becuause its just bad code > > why does this suddenly become desirable when its in a macro? > > > > Or maybe the question should be, does code become less cluttered and > > cleaner when newlines and C keywords are removed ? > > if not why is that done here ? > > if yes why is it done just here ? > > > I agree a macro here doesn't help. My concern wasn't with the check itself, > I agree a file with 100 channels should error out. My concern is that these > files will universally be the result of fuzzing, so I don't want to spam > stderr with messages related to it, nor do I want source/binary size to > increase because of it. > > If you make ff_elog similar to assert (only if NDEBUG is not set), that may > work for the binary size concern, but the source code size is still a > concern. Again, not because it's bad code, but because it's needless since > it only happens for fuzzed samples. +1
On Sat, 28 Jan 2017, Michael Niedermayer wrote: > On Sat, Jan 28, 2017 at 01:26:40AM +0100, Andreas Cadhalpun wrote: >> On 27.01.2017 02:56, Marton Balint wrote: >>> I see 3 problems (wm4 explicitly named them, but I also had them in mind) >>> - Little benefit, yet >>> - Makes the code less clean, more cluttered >>> - Increases binary size >>> >>> The ideas I proposed (use macros, use common / factorized checks for common >>> validatons and errors) might be a good compromise IMHO. Fuzzing thereforce >>> can be done with "debug" builds. >>> >>> Anyway, I am not blocking the patch, just expressing what I would prefer in >>> the long run. >> >> How about the following macro? >> #define FF_RETURN_ERROR(condition, error, log_ctx, ...) { \ >> if (condition) { \ >> if (!CONFIG_SMALL && log_ctx) \ >> av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \ >> return error; \ >> } \ >> } >> >> That could be used with error message: >> FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, AVERROR(ENOSYS), >> s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS) >> >> Or without: >> FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, AVERROR(ENOSYS), NULL) > > I suggest > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { > ff_elog(s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS); > return AVERROR(ENOSYS); > } > > or for the 2nd example: > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) > return AVERROR(ENOSYS); > > > ff_elog() can then be defined to nothing based on CONFIG_SMALL > > the difference between my suggestion and yours is that mine has > new-lines seperating the condition, message and return and that its > self explanatory C code. > > What you do is removing newlines and standard C keywords with a custom > macro that people not working on FFmpeg on a regular basis will have > difficulty understanding > > The macro is similar to writing (minus the C keywords) > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog( > s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); } > > we dont do that becuause its just bad code > why does this suddenly become desirable when its in a macro? > > Or maybe the question should be, does code become less cluttered and > cleaner when newlines and C keywords are removed ? > if not why is that done here ? > if yes why is it done just here ? > If we reduce the number of extra lines (not at any cost), I think that helps. There is also a solution which keeps the traditional C syntax, and is easy to undestand even at first glance. if (st->codecpar->channels > FF_SANE_NB_CHANNELS) return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS); Regards, Marton
On Fri, Jan 27, 2017 at 09:48:05PM -0500, Ronald S. Bultje wrote: > Hi, > > On Fri, Jan 27, 2017 at 9:28 PM, Michael Niedermayer <michaelni@gmx.at> > wrote: > > > On Sat, Jan 28, 2017 at 01:26:40AM +0100, Andreas Cadhalpun wrote: > > > On 27.01.2017 02:56, Marton Balint wrote: > > > > I see 3 problems (wm4 explicitly named them, but I also had them in > > mind) > > > > - Little benefit, yet > > > > - Makes the code less clean, more cluttered > > > > - Increases binary size > > > > > > > > The ideas I proposed (use macros, use common / factorized checks for > > common > > > > validatons and errors) might be a good compromise IMHO. Fuzzing > > thereforce > > > > can be done with "debug" builds. > > > > > > > > Anyway, I am not blocking the patch, just expressing what I would > > prefer in > > > > the long run. > > > > > > How about the following macro? > > > #define FF_RETURN_ERROR(condition, error, log_ctx, ...) { \ > > > if (condition) { \ > > > if (!CONFIG_SMALL && log_ctx) \ > > > av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \ > > > return error; \ > > > } \ > > > } > > > > > > That could be used with error message: > > > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, > > AVERROR(ENOSYS), > > > s, "Too many channels %d > %d\n", > > st->codecpar->channels, FF_SANE_NB_CHANNELS) > > > > > > Or without: > > > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, > > AVERROR(ENOSYS), NULL) > > > > I suggest > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { > > ff_elog(s, "Too many channels %d > %d\n", st->codecpar->channels, > > FF_SANE_NB_CHANNELS); > > return AVERROR(ENOSYS); > > } > > > > or for the 2nd example: > > > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) > > return AVERROR(ENOSYS); > > > > > > ff_elog() can then be defined to nothing based on CONFIG_SMALL > > > > the difference between my suggestion and yours is that mine has > > new-lines seperating the condition, message and return and that its > > self explanatory C code. > > > > What you do is removing newlines and standard C keywords with a custom > > macro that people not working on FFmpeg on a regular basis will have > > difficulty understanding > > > > The macro is similar to writing (minus the C keywords) > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog( > > s, "Too many channels %d > %d\n", st->codecpar->channels, > > FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); } > > > > we dont do that becuause its just bad code > > why does this suddenly become desirable when its in a macro? > > > > Or maybe the question should be, does code become less cluttered and > > cleaner when newlines and C keywords are removed ? > > if not why is that done here ? > > if yes why is it done just here ? > > > I agree a macro here doesn't help. My concern wasn't with the check itself, > I agree a file with 100 channels should error out. My concern is that these > files will universally be the result of fuzzing, so I don't want to spam > stderr with messages related to it, nor do I want source/binary size to > increase because of it. > > If you make ff_elog similar to assert (only if NDEBUG is not set), that may > work for the binary size concern, but the source code size is still a > concern. Again, not because it's bad code, but because it's needless since > it only happens for fuzzed samples. its needless to you, its not needless to me. When i work on fixing issues found by fuzzed samples, crashes, undefined behavior, security issues, having detailed error messages makes it sometimes simpler Having no error messages related to fuzzed samples or only a subset of them will make debuging issues related to fuzzing harder. an issue after an error path for example, its easier if you know which error it was than if you just see a crash and dont even know that an error occured as the crash happens before the app exists with an error of its own. Nothing of that makes it impossible to fix but it makes it cost more time if the specific error path prints no error message I dont think its desirable to make debuging issues related to fuzzing harder. Nor to make me spend more time per bugfix than otherwise needed, even if that is maybe not a large factor its not nothing [...]
On 1/28/17, Michael Niedermayer <michaelni@gmx.at> wrote: > On Fri, Jan 27, 2017 at 09:48:05PM -0500, Ronald S. Bultje wrote: >> Hi, >> >> On Fri, Jan 27, 2017 at 9:28 PM, Michael Niedermayer <michaelni@gmx.at> >> wrote: >> >> > On Sat, Jan 28, 2017 at 01:26:40AM +0100, Andreas Cadhalpun wrote: >> > > On 27.01.2017 02:56, Marton Balint wrote: >> > > > I see 3 problems (wm4 explicitly named them, but I also had them in >> > mind) >> > > > - Little benefit, yet >> > > > - Makes the code less clean, more cluttered >> > > > - Increases binary size >> > > > >> > > > The ideas I proposed (use macros, use common / factorized checks >> > > > for >> > common >> > > > validatons and errors) might be a good compromise IMHO. Fuzzing >> > thereforce >> > > > can be done with "debug" builds. >> > > > >> > > > Anyway, I am not blocking the patch, just expressing what I would >> > prefer in >> > > > the long run. >> > > >> > > How about the following macro? >> > > #define FF_RETURN_ERROR(condition, error, log_ctx, ...) { \ >> > > if (condition) { \ >> > > if (!CONFIG_SMALL && log_ctx) \ >> > > av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \ >> > > return error; \ >> > > } \ >> > > } >> > > >> > > That could be used with error message: >> > > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, >> > AVERROR(ENOSYS), >> > > s, "Too many channels %d > %d\n", >> > st->codecpar->channels, FF_SANE_NB_CHANNELS) >> > > >> > > Or without: >> > > FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, >> > AVERROR(ENOSYS), NULL) >> > >> > I suggest >> > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { >> > ff_elog(s, "Too many channels %d > %d\n", st->codecpar->channels, >> > FF_SANE_NB_CHANNELS); >> > return AVERROR(ENOSYS); >> > } >> > >> > or for the 2nd example: >> > >> > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) >> > return AVERROR(ENOSYS); >> > >> > >> > ff_elog() can then be defined to nothing based on CONFIG_SMALL >> > >> > the difference between my suggestion and yours is that mine has >> > new-lines seperating the condition, message and return and that its >> > self explanatory C code. >> > >> > What you do is removing newlines and standard C keywords with a custom >> > macro that people not working on FFmpeg on a regular basis will have >> > difficulty understanding >> > >> > The macro is similar to writing (minus the C keywords) >> > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog( >> > s, "Too many channels %d > %d\n", st->codecpar->channels, >> > FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); } >> > >> > we dont do that becuause its just bad code >> > why does this suddenly become desirable when its in a macro? >> > >> > Or maybe the question should be, does code become less cluttered and >> > cleaner when newlines and C keywords are removed ? >> > if not why is that done here ? >> > if yes why is it done just here ? >> >> >> I agree a macro here doesn't help. My concern wasn't with the check >> itself, >> I agree a file with 100 channels should error out. My concern is that >> these >> files will universally be the result of fuzzing, so I don't want to spam >> stderr with messages related to it, nor do I want source/binary size to >> increase because of it. >> > >> If you make ff_elog similar to assert (only if NDEBUG is not set), that >> may >> work for the binary size concern, but the source code size is still a >> concern. Again, not because it's bad code, but because it's needless >> since >> it only happens for fuzzed samples. > > its needless to you, its not needless to me. When i work on fixing > issues found by fuzzed samples, crashes, undefined behavior, security > issues, having detailed error messages makes it sometimes simpler > > Having no error messages related to fuzzed samples or only a subset of > them will make debuging issues related to fuzzing harder. > > an issue after an error path for example, its easier if you > know which error it was than if you just see a crash and dont even > know that an error occured as the crash happens before the app > exists with an error of its own. > Nothing of that makes it impossible to fix but it makes it cost more > time if the specific error path prints no error message > > I dont think its desirable to make debuging issues related to fuzzing > harder. Nor to make me spend more time per bugfix than otherwise needed, > even if that is maybe not a large factor its not nothing For debugging crashes you use crash file and valgrind. You do not spread code with av_log messages.
On 28.01.2017 03:48, Ronald S. Bultje wrote: > I agree a macro here doesn't help. My concern wasn't with the check itself, > I agree a file with 100 channels should error out. My concern is that these > files will universally be the result of fuzzing, so I don't want to spam > stderr with messages related to it, nor do I want source/binary size to > increase because of it. > > If you make ff_elog similar to assert (only if NDEBUG is not set), that may > work for the binary size concern, but the source code size is still a > concern. Again, not because it's bad code, but because it's needless since > it only happens for fuzzed samples. You claim that, but it's impossible to prove and thus likely wrong. Also it's quite arbitrary that you object to this log message, while e.g. the following has been there for years: if (s->nb_streams == ASF_MAX_STREAMS) { av_log(s, AV_LOG_ERROR, "too many streams\n"); return AVERROR(EINVAL); } Unless you can come up with objective criteria, when to add log messages and when not, this topic is going to be a pointless waste of time. Best regards, Andreas
On 28.01.2017 12:44, Marton Balint wrote: > If we reduce the number of extra lines (not at any cost), I think that helps. > There is also a solution which keeps the traditional C syntax, and is easy to undestand even at first glance. > > if (st->codecpar->channels > FF_SANE_NB_CHANNELS) > return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS); How would you define ff_elog for this to work? Best regards, Andreas
On 1/29/17, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > On 28.01.2017 12:44, Marton Balint wrote: >> If we reduce the number of extra lines (not at any cost), I think that >> helps. >> There is also a solution which keeps the traditional C syntax, and is easy >> to undestand even at first glance. >> >> if (st->codecpar->channels > FF_SANE_NB_CHANNELS) >> return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n", >> st->codecpar->channels, FF_SANE_NB_CHANNELS); > > How would you define ff_elog for this to work? I'm maintainer of this file, and I'm fed up with this nuisance conversation. I'm against log message.
On Sun, 29 Jan 2017, Andreas Cadhalpun wrote: > On 28.01.2017 12:44, Marton Balint wrote: >> If we reduce the number of extra lines (not at any cost), I think that helps. >> There is also a solution which keeps the traditional C syntax, and is easy to undestand even at first glance. >> >> if (st->codecpar->channels > FF_SANE_NB_CHANNELS) >> return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS); > > How would you define ff_elog for this to work? > static inline int ff_elog(int error, void *log_ctx, const char *fmt, ...) { if (!CONFIG_SMALL) { va_list vl; va_start(vl, fmt); av_vlog(log_ctx, AV_LOG_ERROR, fmt, vl); va_end(vl); } return error; } Regards, Marton
On 29.01.2017 04:02, Marton Balint wrote: > > On Sun, 29 Jan 2017, Andreas Cadhalpun wrote: > >> On 28.01.2017 12:44, Marton Balint wrote: >>> If we reduce the number of extra lines (not at any cost), I think that helps. >>> There is also a solution which keeps the traditional C syntax, and is easy to undestand even at first glance. >>> >>> if (st->codecpar->channels > FF_SANE_NB_CHANNELS) >>> return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS); >> >> How would you define ff_elog for this to work? >> > > static inline int ff_elog(int error, void *log_ctx, const char *fmt, ...) { > if (!CONFIG_SMALL) { > va_list vl; > va_start(vl, fmt); > av_vlog(log_ctx, AV_LOG_ERROR, fmt, vl); > va_end(vl); > } > return error; > } OK. I'd be fine with using it, if people prefer this way. Best regards, Andreas
diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c index 782d1dfbfb..3386497682 100644 --- a/libavformat/nistspheredec.c +++ b/libavformat/nistspheredec.c @@ -21,6 +21,7 @@ #include "libavutil/avstring.h" #include "libavutil/intreadwrite.h" +#include "libavcodec/internal.h" #include "avformat.h" #include "internal.h" #include "pcm.h" @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) return 0; } else if (!memcmp(buffer, "channel_count", 13)) { sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); + 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); + } } else if (!memcmp(buffer, "sample_byte_format", 18)) { sscanf(buffer, "%*s %*s %31s", format); @@ -109,6 +115,11 @@ static int nist_read_header(AVFormatContext *s) sscanf(buffer, "%*s %*s %"SCNd64, &st->duration); } else if (!memcmp(buffer, "sample_n_bytes", 14)) { sscanf(buffer, "%*s %*s %"SCNd32, &bps); + if (bps > (INT_MAX / FF_SANE_NB_CHANNELS) >> 3) { + av_log(s, AV_LOG_ERROR, "Too many bytes per sample %d > %d\n", + bps, (INT_MAX / FF_SANE_NB_CHANNELS) >> 3); + return AVERROR_INVALIDDATA; + } } else if (!memcmp(buffer, "sample_rate", 11)) { sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->sample_rate); } else if (!memcmp(buffer, "sample_sig_bits", 15)) {
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavformat/nistspheredec.c | 11 +++++++++++ 1 file changed, 11 insertions(+)