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

[Michael Niedermayer]

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

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170128/5656cc8a/attachment.sig>