[FFmpeg-devel] [PATCH 1/7] avutil: add FF_BAIL_ON_OVERFLOW

[Paul B Mahol]

On 12/20/16, wm4 <nfxjfg at googlemail.com> wrote:
> On Mon, 19 Dec 2016 23:36:11 +0100
> Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
>
>> On 16.12.2016 17:22, wm4 wrote:
>> > On Fri, 16 Dec 2016 03:32:07 +0100
>> > Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
>> >
>> >> Suggested-by: Rodger Combs <rodger.combs at gmail.com>
>> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> >> ---
>> >>  libavutil/common.h | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/libavutil/common.h b/libavutil/common.h
>> >> index 8142b31..00b7504 100644
>> >> --- a/libavutil/common.h
>> >> +++ b/libavutil/common.h
>> >> @@ -99,6 +99,8 @@
>> >>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a=
>> >> SWAP_tmp;}while(0)
>> >>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>> >>
>> >> +#define FF_BAIL_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR,
>> >> "Overflow check failed: " #x"\n"); return AVERROR_INVALIDDATA;}
>> >> +
>> >>  /* misc math functions */
>> >>
>> >>  #ifdef HAVE_AV_CONFIG_H
>> >
>> > Are you sure we need the message?
>>
>> Yes, since such an overflow could just be a sign of a limitation in our
>> framework (think of bit_rate being int32_t) and does not necessarily mean
>> that the sample is invalid.
>>
>> > It's quite ugly.
>>
>> Do you have any suggestions for improving it?
>
> I'm pretty much against such macros for rather specific use-cases, and
> putting them into a public headers. I'm thinking it'd be better to
> actually provide overflow-checking primitives, and I also don't think
> every overflow has to be logged. Almost all of these cases happen only
> when fuzzing anyway.

Yes, if one wants logs let them compile with sanitizer options.