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

Marton Balint cus at passwd.hu
Sat Jan 28 13:44:31 EET 2017


On Sat, 28 Jan 2017, Michael Niedermayer 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 ?
>

If we reduce the number of extra lines (not at any cost), I think that 
helps. There is also a solution which keeps the traditional C syntax, and 
is easy to undestand even at first glance.

if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
     return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n", st->codecpar->channels, FF_SANE_NB_CHANNELS);

Regards,
Marton


More information about the ffmpeg-devel mailing list