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

[Paul B Mahol]

On 1/28/17, Michael Niedermayer <michaelni at 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 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

For debugging crashes you use crash file and valgrind. You do not spread code
with av_log messages.