| Message ID | 8f290505-70a1-d5b0-cbcf-96522ab12ce4@googlemail.com |
|---|---|
| State | New |
| Headers | show |
On 1/6/2017 4:46 PM, Andreas Cadhalpun wrote: > Suggested-by: Rodger Combs <rodger.combs@gmail.com> > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > > Changed the name as suggested by wm4 and the return value as suggested > by Muhammad Faiz. > There are also two new overflow checks at the end of the series. This is a good chance to introduce the gcc overflow check builtins. See https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html, they use hardware instructions when possible, like x86's Jump on Overflow. The idea would be to use __builtin_mul_overflow_p(). For example (untested): #if AV_GCC_VERSION_AT_LEAST(5,1) #define av_builtin_mul_overflow_p(a, b) __builtin_mul_overflow_p(a, b, (int) 0) #else #define av_builtin_mul_overflow_p(a, b) ((a) > INT_MAX / (b))) #endif It can also be used all across the codebase, not just for these checks. > > --- > libavutil/common.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavutil/common.h b/libavutil/common.h > index 8142b31fdb..6d795a353a 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_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);} Printing an error unconditionally seems like log bloat. We do all kinds of sanity checks on demuxers and simply return INVALIDDATA without printing anything if they fail. Maybe we should do the same here and let the demuxer choose to print an error or not. > + > /* misc math functions */ > > #ifdef HAVE_AV_CONFIG_H >
On Fri, Jan 06, 2017 at 09:11:10PM -0300, James Almer wrote: > On 1/6/2017 4:46 PM, Andreas Cadhalpun wrote: > > Suggested-by: Rodger Combs <rodger.combs@gmail.com> > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > > --- > > > > Changed the name as suggested by wm4 and the return value as suggested > > by Muhammad Faiz. > > There are also two new overflow checks at the end of the series. > > This is a good chance to introduce the gcc overflow check builtins. > See https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html, they > use hardware instructions when possible, like x86's Jump on Overflow. > > The idea would be to use __builtin_mul_overflow_p(). For example (untested): > > #if AV_GCC_VERSION_AT_LEAST(5,1) > #define av_builtin_mul_overflow_p(a, b) __builtin_mul_overflow_p(a, b, (int) 0) > #else > #define av_builtin_mul_overflow_p(a, b) ((a) > INT_MAX / (b))) > #endif > > It can also be used all across the codebase, not just for these checks. > > > > > --- > > libavutil/common.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavutil/common.h b/libavutil/common.h > > index 8142b31fdb..6d795a353a 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_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);} > > Printing an error unconditionally seems like log bloat. We do all kinds of > sanity checks on demuxers and simply return INVALIDDATA without printing > anything if they fail. > Maybe we should do the same here and let the demuxer choose to print an > error or not. error messages help debuging and thus maintaining code. [...]
On 1/7/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Jan 06, 2017 at 09:11:10PM -0300, James Almer wrote: >> On 1/6/2017 4:46 PM, Andreas Cadhalpun wrote: >> > Suggested-by: Rodger Combs <rodger.combs@gmail.com> >> > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> > --- >> > >> > Changed the name as suggested by wm4 and the return value as suggested >> > by Muhammad Faiz. >> > There are also two new overflow checks at the end of the series. >> >> This is a good chance to introduce the gcc overflow check builtins. >> See https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html, >> they >> use hardware instructions when possible, like x86's Jump on Overflow. >> >> The idea would be to use __builtin_mul_overflow_p(). For example >> (untested): >> >> #if AV_GCC_VERSION_AT_LEAST(5,1) >> #define av_builtin_mul_overflow_p(a, b) __builtin_mul_overflow_p(a, b, >> (int) 0) >> #else >> #define av_builtin_mul_overflow_p(a, b) ((a) > INT_MAX / (b))) >> #endif >> >> It can also be used all across the codebase, not just for these checks. >> > >> > >> > --- >> > libavutil/common.h | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/libavutil/common.h b/libavutil/common.h >> > index 8142b31fdb..6d795a353a 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_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, >> > "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);} >> >> Printing an error unconditionally seems like log bloat. We do all kinds >> of >> sanity checks on demuxers and simply return INVALIDDATA without printing >> anything if they fail. >> Maybe we should do the same here and let the demuxer choose to print an >> error or not. > > error messages help debuging and thus maintaining code. > Not really. Expecially in this case.
> On Jan 7, 2017, at 02:36, Paul B Mahol <onemda@gmail.com> wrote: > > On 1/7/17, Michael Niedermayer <michael@niedermayer.cc <mailto:michael@niedermayer.cc>> wrote: >> On Fri, Jan 06, 2017 at 09:11:10PM -0300, James Almer wrote: >>> On 1/6/2017 4:46 PM, Andreas Cadhalpun wrote: >>>> Suggested-by: Rodger Combs <rodger.combs@gmail.com> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >>>> --- >>>> >>>> Changed the name as suggested by wm4 and the return value as suggested >>>> by Muhammad Faiz. >>>> There are also two new overflow checks at the end of the series. >>> >>> This is a good chance to introduce the gcc overflow check builtins. >>> See https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html, >>> they >>> use hardware instructions when possible, like x86's Jump on Overflow. >>> >>> The idea would be to use __builtin_mul_overflow_p(). For example >>> (untested): >>> >>> #if AV_GCC_VERSION_AT_LEAST(5,1) >>> #define av_builtin_mul_overflow_p(a, b) __builtin_mul_overflow_p(a, b, >>> (int) 0) >>> #else >>> #define av_builtin_mul_overflow_p(a, b) ((a) > INT_MAX / (b))) >>> #endif >>> >>> It can also be used all across the codebase, not just for these checks. >>> >> >>>> >>>> --- >>>> libavutil/common.h | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/libavutil/common.h b/libavutil/common.h >>>> index 8142b31fdb..6d795a353a 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_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, >>>> "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);} >>> >>> Printing an error unconditionally seems like log bloat. We do all kinds >>> of >>> sanity checks on demuxers and simply return INVALIDDATA without printing >>> anything if they fail. >>> Maybe we should do the same here and let the demuxer choose to print an >>> error or not. >> >> error messages help debuging and thus maintaining code. >> > > Not really. Expecially in this case. Eh, I generally prefer a print-and-return over just returning an error code in cases where the source is ambiguous. There are a lot of cases currently where in e.g. ffmpeg CLI, the code just gets bubbled up and printed, potentially with the name of the codec it came from, but no further information about the source, so it can be difficult to track down exactly where the failure occurred. Printing a message can be a fair bit more clear. This seems like something it would be worth talking about and deciding on in general, though: as a rule, should we try to print errors about specific failures like this? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
diff --git a/libavutil/common.h b/libavutil/common.h index 8142b31fdb..6d795a353a 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_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, "Overflow check failed: " #x"\n"); return AVERROR(ERANGE);} + /* misc math functions */ #ifdef HAVE_AV_CONFIG_H
Suggested-by: Rodger Combs <rodger.combs@gmail.com> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- Changed the name as suggested by wm4 and the return value as suggested by Muhammad Faiz. There are also two new overflow checks at the end of the series. --- libavutil/common.h | 2 ++ 1 file changed, 2 insertions(+)