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

[wm4]

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.

> 
> > Also maybe call it "FF_RETURN_ON_OVERFLOW".  
> 
> That sounds a bit better, so changed locally.