On Fri, 27 Jan 2017 21:48:05 -0500
"Ronald S. Bultje" <rsbul...@gmail.com> wrote:
> Hi,
>
> On Fri, Jan 27, 2017 at 9:28 PM, Michael Niedermayer <michae...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
