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

[Michael Niedermayer]

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 ?

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

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/920316df/attachment.sig>