[FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Fri, Jan 27, 2017 at 9:28 PM, Michael Niedermayer <michaelni at 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