Re: [FFmpeg-devel] [PATCH 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators

[Clément Bœsch]

On Sun, Nov 01, 2015 at 07:29:36PM -0500, Ganesh Ajjanagadde wrote:
[...]
> I believe vf_palettegen and libavformat/subtitles are the only ones
> lacking review, but may be mistaken.
> 

Those are probably fine.

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators

[Ganesh Ajjanagadde]

On Tue, Nov 3, 2015 at 5:54 AM, Clément Bœsch  wrote:
> On Sun, Nov 01, 2015 at 07:29:36PM -0500, Ganesh Ajjanagadde wrote:
> [...]
>> I believe vf_palettegen and libavformat/subtitles are the only ones
>> lacking review, but may be mistaken.
>>
>
> Those are probably fine.

Pushed, thanks.

>
> --
> Clément B.
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators

[Ganesh Ajjanagadde]

FFDIFFSIGN was created explicitly for this purpose, since the common
return a - b idiom is unsafe regarding overflow on signed integers. It
optimizes to branchless code on common compilers.

FFDIFFSIGN also has the subjective benefit of being easier to read due
to lack of ternary operators.

Tested with FATE.

Things not covered by this are unsigned integers, for which overflows
are well defined, and also places where overflow is clearly impossible,
e.g an instance where the a - b was being done on 24 bit values.
I can convert some of these to FFDIFFSIGN if people find it a
readability improvement.

Signed-off-by: Ganesh Ajjanagadde 
---
 cmdutils.c  | 2 +-
 cmdutils_opencl.c   | 2 +-
 ffmpeg.c| 3 +--
 libavcodec/aacsbr_template.c| 2 +-
 libavcodec/motion_est.c | 2 +-
 libavfilter/f_sendcmd.c | 8 +++-
 libavfilter/vf_deshake.c| 3 +--
 libavfilter/vf_palettegen.c | 2 +-
 libavfilter/vf_removegrain.c| 5 +
 libavformat/subtitles.c | 9 +++--
 libswresample/swresample-test.c | 2 +-
 11 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/cmdutils.c b/cmdutils.c
index e3e9891..41daa95 100644
--- a/cmdutils.c
+++ b/cmdutils.c
@@ -1421,7 +1421,7 @@ static int compare_codec_desc(const void *a, const void 
*b)
 const AVCodecDescriptor * const *da = a;
 const AVCodecDescriptor * const *db = b;
 
-return (*da)->type != (*db)->type ? (*da)->type - (*db)->type :
+return (*da)->type != (*db)->type ? FFDIFFSIGN((*da)->type, (*db)->type) :
strcmp((*da)->name, (*db)->name);
 }
 
diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c
index d9095b6..5621ebc 100644
--- a/cmdutils_opencl.c
+++ b/cmdutils_opencl.c
@@ -206,7 +206,7 @@ end:
 
 static int compare_ocl_device_desc(const void *a, const void *b)
 {
-return ((const OpenCLDeviceBenchmark*)a)->runtime - ((const 
OpenCLDeviceBenchmark*)b)->runtime;
+return FFDIFFSIGN((const OpenCLDeviceBenchmark*)a->runtime , (const 
OpenCLDeviceBenchmark*)b->runtime);
 }
 
 int opt_opencl_bench(void *optctx, const char *opt, const char *arg)
diff --git a/ffmpeg.c b/ffmpeg.c
index f8b071a..d3b8c4d 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -2578,8 +2578,7 @@ static InputStream *get_input_stream(OutputStream *ost)
 
 static int compare_int64(const void *a, const void *b)
 {
-int64_t va = *(int64_t *)a, vb = *(int64_t *)b;
-return va < vb ? -1 : va > vb ? +1 : 0;
+return FFDIFFSIGN(*(const int64_t *)a, *(const int64_t *)b);
 }
 
 static int init_output_stream(OutputStream *ost, char *error, int error_len)
diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
index d31b71e..1e6f149 100644
--- a/libavcodec/aacsbr_template.c
+++ b/libavcodec/aacsbr_template.c
@@ -104,7 +104,7 @@ av_cold void 
AAC_RENAME(ff_aac_sbr_ctx_close)(SpectralBandReplication *sbr)
 
 static int qsort_comparison_function_int16(const void *a, const void *b)
 {
-return *(const int16_t *)a - *(const int16_t *)b;
+return FFDIFFSIGN(*(const int16_t *)a , *(const int16_t *)b);
 }
 
 static inline int in_table_int16(const int16_t *table, int last_el, int16_t 
needle)
diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c
index 9f71568..8560819 100644
--- a/libavcodec/motion_est.c
+++ b/libavcodec/motion_est.c
@@ -73,7 +73,7 @@ static int minima_cmp(const void *a, const void *b){
 const Minima *da = (const Minima *) a;
 const Minima *db = (const Minima *) b;
 
-return da->height - db->height;
+return FFDIFFSIGN(da->height , db->height);
 }
 
 #define FLAG_QPEL   1 //must be 1
diff --git a/libavfilter/f_sendcmd.c b/libavfilter/f_sendcmd.c
index 37aedc5..f06773a 100644
--- a/libavfilter/f_sendcmd.c
+++ b/libavfilter/f_sendcmd.c
@@ -364,11 +364,9 @@ static int cmp_intervals(const void *a, const void *b)
 {
 const Interval *i1 = a;
 const Interval *i2 = b;
-int64_t ts_diff = i1->start_ts - i2->start_ts;
-int ret;
-
-ret = ts_diff > 0 ? 1 : ts_diff < 0 ? -1 : 0;
-return ret == 0 ? i1->index - i2->index : ret;
+if (i1->start_ts == i2->start_ts)
+return FFDIFFSIGN(i1->index, i2->index);
+return FFDIFFSIGN(i1->start_ts, i2->start_ts);
 }
 
 static av_cold int init(AVFilterContext *ctx)
diff --git a/libavfilter/vf_deshake.c b/libavfilter/vf_deshake.c
index e32436d..79fcc20 100644
--- a/libavfilter/vf_deshake.c
+++ b/libavfilter/vf_deshake.c
@@ -94,8 +94,7 @@ AVFILTER_DEFINE_CLASS(deshake);
 
 static int cmp(const void *a, const void *b)
 {
-const double va = *(const double *)a, vb = *(const double *)b;
-return va < vb ? -1 : ( va > vb ? 1 : 0 );
+return FFDIFFSIGN(*(const double *)a, *(const double *)b);
 }
 
 /**
diff --git a/libavfilter/vf_palettegen.c b/libavfilter/vf_palettegen.c
index df57c10..fccc5ca 100644
--- a/libavfilter/vf_palettegen.c
+++ 

Re: [FFmpeg-devel] [PATCH 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators

[Ganesh Ajjanagadde]

On Sun, Nov 1, 2015 at 6:26 PM, Michael Niedermayer
 wrote:
> On Sun, Nov 01, 2015 at 12:19:48PM -0500, Ganesh Ajjanagadde wrote:
>> FFDIFFSIGN was created explicitly for this purpose, since the common
>> return a - b idiom is unsafe regarding overflow on signed integers. It
>> optimizes to branchless code on common compilers.
>>
>> FFDIFFSIGN also has the subjective benefit of being easier to read due
>> to lack of ternary operators.
>>
>> Tested with FATE.
>> 
>> Things not covered by this are unsigned integers, for which overflows
>> are well defined, and also places where overflow is clearly impossible,
>> e.g an instance where the a - b was being done on 24 bit values.
>> I can convert some of these to FFDIFFSIGN if people find it a
>> readability improvement.
>>
>> Signed-off-by: Ganesh Ajjanagadde 
>> ---
>>  cmdutils.c  | 2 +-
>>  cmdutils_opencl.c   | 2 +-
>>  ffmpeg.c| 3 +--
>>  libavcodec/aacsbr_template.c| 2 +-
>>  libavcodec/motion_est.c | 2 +-
>>  libavfilter/f_sendcmd.c | 8 +++-
>>  libavfilter/vf_deshake.c| 3 +--
>>  libavfilter/vf_palettegen.c | 2 +-
>>  libavfilter/vf_removegrain.c| 5 +
>>  libavformat/subtitles.c | 9 +++--
>>  libswresample/swresample-test.c | 2 +-
>>  11 files changed, 15 insertions(+), 25 deletions(-)
>>
>> diff --git a/cmdutils.c b/cmdutils.c
>> index e3e9891..41daa95 100644
>> --- a/cmdutils.c
>> +++ b/cmdutils.c
>> @@ -1421,7 +1421,7 @@ static int compare_codec_desc(const void *a, const 
>> void *b)
>>  const AVCodecDescriptor * const *da = a;
>>  const AVCodecDescriptor * const *db = b;
>>
>> -return (*da)->type != (*db)->type ? (*da)->type - (*db)->type :
>> +return (*da)->type != (*db)->type ? FFDIFFSIGN((*da)->type, 
>> (*db)->type) :
>
> Overflow is probably impossible for all "normal" enums,
> you wrote above that you omitted cases where its impossible to
> overflow, so you maybe want to omit this too

This one was deliberate as a safety measure for future work with the
codec types: implementers should be free to use the full enum range,
and I don't want there to be safety issues. This is an instance of
what I meant by "defensive programming". Strongly prefer not to
change.

>
>
>
>> strcmp((*da)->name, (*db)->name);
>>  }
>>
>> diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c
>> index d9095b6..5621ebc 100644
>> --- a/cmdutils_opencl.c
>> +++ b/cmdutils_opencl.c
>> @@ -206,7 +206,7 @@ end:
>>
>>  static int compare_ocl_device_desc(const void *a, const void *b)
>>  {
>> -return ((const OpenCLDeviceBenchmark*)a)->runtime - ((const 
>> OpenCLDeviceBenchmark*)b)->runtime;
>> +return FFDIFFSIGN((const OpenCLDeviceBenchmark*)a->runtime , (const 
>> OpenCLDeviceBenchmark*)b->runtime);
>>  }
>
> nitpick: The reading of the 2 values should be on seperate lines
> to make this a bt more readable

ok, as you may see I favored the one line return style due to its
terseness. I personally don't mind either way and you favor the other,
so changed.

>
>
>>
>>  int opt_opencl_bench(void *optctx, const char *opt, const char *arg)
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index f8b071a..d3b8c4d 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -2578,8 +2578,7 @@ static InputStream *get_input_stream(OutputStream *ost)
>>
>>  static int compare_int64(const void *a, const void *b)
>>  {
>> -int64_t va = *(int64_t *)a, vb = *(int64_t *)b;
>> -return va < vb ? -1 : va > vb ? +1 : 0;
>> +return FFDIFFSIGN(*(const int64_t *)a, *(const int64_t *)b);
>
> ok
>
>
>>  }
>>
>>  static int init_output_stream(OutputStream *ost, char *error, int error_len)
>> diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
>> index d31b71e..1e6f149 100644
>> --- a/libavcodec/aacsbr_template.c
>> +++ b/libavcodec/aacsbr_template.c
>> @@ -104,7 +104,7 @@ av_cold void 
>> AAC_RENAME(ff_aac_sbr_ctx_close)(SpectralBandReplication *sbr)
>>
>>  static int qsort_comparison_function_int16(const void *a, const void *b)
>>  {
>> -return *(const int16_t *)a - *(const int16_t *)b;
>> +return FFDIFFSIGN(*(const int16_t *)a , *(const int16_t *)b);
>
> a int16 difference cannot overflow

right, changed.

>
>
>>  }
>>
>>  static inline int in_table_int16(const int16_t *table, int last_el, int16_t 
>> needle)
>> diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c
>> index 9f71568..8560819 100644
>> --- a/libavcodec/motion_est.c
>> +++ b/libavcodec/motion_est.c
>> @@ -73,7 +73,7 @@ static int minima_cmp(const void *a, const void *b){
>>  const Minima *da = (const Minima *) a;
>>  const Minima *db = (const Minima *) b;
>>
>> -return da->height - db->height;
>> +return FFDIFFSIGN(da->height , db->height);
>>  }
>
> this may be speed relevant,
> is this faster or slower or same speed than 

Re: [FFmpeg-devel] [PATCH 2/2] all: use FFDIFFSIGN to resolve possible undefined behavior in comparators

[Michael Niedermayer]

On Sun, Nov 01, 2015 at 12:19:48PM -0500, Ganesh Ajjanagadde wrote:
> FFDIFFSIGN was created explicitly for this purpose, since the common
> return a - b idiom is unsafe regarding overflow on signed integers. It
> optimizes to branchless code on common compilers.
> 
> FFDIFFSIGN also has the subjective benefit of being easier to read due
> to lack of ternary operators.
> 
> Tested with FATE.
> 
> Things not covered by this are unsigned integers, for which overflows
> are well defined, and also places where overflow is clearly impossible,
> e.g an instance where the a - b was being done on 24 bit values.
> I can convert some of these to FFDIFFSIGN if people find it a
> readability improvement.
> 
> Signed-off-by: Ganesh Ajjanagadde 
> ---
>  cmdutils.c  | 2 +-
>  cmdutils_opencl.c   | 2 +-
>  ffmpeg.c| 3 +--
>  libavcodec/aacsbr_template.c| 2 +-
>  libavcodec/motion_est.c | 2 +-
>  libavfilter/f_sendcmd.c | 8 +++-
>  libavfilter/vf_deshake.c| 3 +--
>  libavfilter/vf_palettegen.c | 2 +-
>  libavfilter/vf_removegrain.c| 5 +
>  libavformat/subtitles.c | 9 +++--
>  libswresample/swresample-test.c | 2 +-
>  11 files changed, 15 insertions(+), 25 deletions(-)
> 
> diff --git a/cmdutils.c b/cmdutils.c
> index e3e9891..41daa95 100644
> --- a/cmdutils.c
> +++ b/cmdutils.c
> @@ -1421,7 +1421,7 @@ static int compare_codec_desc(const void *a, const void 
> *b)
>  const AVCodecDescriptor * const *da = a;
>  const AVCodecDescriptor * const *db = b;
>  
> -return (*da)->type != (*db)->type ? (*da)->type - (*db)->type :
> +return (*da)->type != (*db)->type ? FFDIFFSIGN((*da)->type, (*db)->type) 
> :

Overflow is probably impossible for all "normal" enums,
you wrote above that you omitted cases where its impossible to
overflow, so you maybe want to omit this too



> strcmp((*da)->name, (*db)->name);
>  }
>  
> diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c
> index d9095b6..5621ebc 100644
> --- a/cmdutils_opencl.c
> +++ b/cmdutils_opencl.c
> @@ -206,7 +206,7 @@ end:
>  
>  static int compare_ocl_device_desc(const void *a, const void *b)
>  {
> -return ((const OpenCLDeviceBenchmark*)a)->runtime - ((const 
> OpenCLDeviceBenchmark*)b)->runtime;
> +return FFDIFFSIGN((const OpenCLDeviceBenchmark*)a->runtime , (const 
> OpenCLDeviceBenchmark*)b->runtime);
>  }

nitpick: The reading of the 2 values should be on seperate lines
to make this a bt more readable


>  
>  int opt_opencl_bench(void *optctx, const char *opt, const char *arg)
> diff --git a/ffmpeg.c b/ffmpeg.c
> index f8b071a..d3b8c4d 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -2578,8 +2578,7 @@ static InputStream *get_input_stream(OutputStream *ost)
>  
>  static int compare_int64(const void *a, const void *b)
>  {
> -int64_t va = *(int64_t *)a, vb = *(int64_t *)b;
> -return va < vb ? -1 : va > vb ? +1 : 0;
> +return FFDIFFSIGN(*(const int64_t *)a, *(const int64_t *)b);

ok


>  }
>  
>  static int init_output_stream(OutputStream *ost, char *error, int error_len)
> diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
> index d31b71e..1e6f149 100644
> --- a/libavcodec/aacsbr_template.c
> +++ b/libavcodec/aacsbr_template.c
> @@ -104,7 +104,7 @@ av_cold void 
> AAC_RENAME(ff_aac_sbr_ctx_close)(SpectralBandReplication *sbr)
>  
>  static int qsort_comparison_function_int16(const void *a, const void *b)
>  {
> -return *(const int16_t *)a - *(const int16_t *)b;
> +return FFDIFFSIGN(*(const int16_t *)a , *(const int16_t *)b);

a int16 difference cannot overflow


>  }
>  
>  static inline int in_table_int16(const int16_t *table, int last_el, int16_t 
> needle)
> diff --git a/libavcodec/motion_est.c b/libavcodec/motion_est.c
> index 9f71568..8560819 100644
> --- a/libavcodec/motion_est.c
> +++ b/libavcodec/motion_est.c
> @@ -73,7 +73,7 @@ static int minima_cmp(const void *a, const void *b){
>  const Minima *da = (const Minima *) a;
>  const Minima *db = (const Minima *) b;
>  
> -return da->height - db->height;
> +return FFDIFFSIGN(da->height , db->height);
>  }

this may be speed relevant,
is this faster or slower or same speed than before ?
also i suspect overflow is not be an issue here


>  
>  #define FLAG_QPEL   1 //must be 1
> diff --git a/libavfilter/f_sendcmd.c b/libavfilter/f_sendcmd.c
> index 37aedc5..f06773a 100644
> --- a/libavfilter/f_sendcmd.c
> +++ b/libavfilter/f_sendcmd.c
> @@ -364,11 +364,9 @@ static int cmp_intervals(const void *a, const void *b)
>  {
>  const Interval *i1 = a;
>  const Interval *i2 = b;
> -int64_t ts_diff = i1->start_ts - i2->start_ts;
> -int ret;
> -
> -ret = ts_diff > 0 ? 1 : ts_diff < 0 ? -1 : 0;
> -return ret == 0 ? i1->index - i2->index : ret;
> +if (i1->start_ts == i2->start_ts)
> +return