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

[wm4]

On Fri, 27 Jan 2017 21:48:05 -0500
"Ronald S. Bultje" <rsbultje at gmail.com> wrote:

> 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.

+1