Re: [FFmpeg-devel] [PATCH 2/2] configure: Use -fno-sanitize-recover

Re: [FFmpeg-devel] [PATCH 2/2] configure: Use -fno-sanitize-recover

[Vitaly Buka via ffmpeg-devel]

My confusion here is that it looks like ffmpeg developers care about UB, I
see from time to time large cleanups, but there are a bunch of unfixed
reports.
Maybe forcing no-recover by default will improve this situation?


On Fri, Jan 17, 2025 at 11:57 AM Frank Plowman <post@frankplowman.com>
wrote:

> On 16/01/2025 19:12, Vitaly Buka via ffmpeg-devel wrote:
> > UBSAN by default is just prints a mesage and
> > moves on. This hides a few UBs in fate-suite.
> >
> > Signed-off-by: Vitaly Buka <vitalybuka@google.com>
> > ---
> >  configure                     | 4 ++--
> >  libavcodec/aacenc_pred.c      | 1 +
> >  libavcodec/ffv1dec.c          | 1 +
> >  libavcodec/ffv1enc_template.c | 1 +
> >  libavcodec/get_bits.h         | 1 +
> >  libavcodec/indeo3.c           | 2 +-
> >  libavcodec/motion_est.c       | 1 +
> >  libavcodec/mss2dsp.c          | 1 +
> >  libavcodec/opus/dec.c         | 1 +
> >  libavcodec/snow.h             | 1 +
> >  libavcodec/svq1enc.c          | 1 +
> >  libavfilter/vf_curves.c       | 1 +
> >  libavfilter/vf_overlay.c      | 1 +
> >  libavformat/mov.c             | 1 +
> >  libswscale/input.c            | 6 ++++++
> >  libswscale/output.c           | 4 ++++
> >  libswscale/swscale_unscaled.c | 3 +++
> >  17 files changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 3a1e72e1c6..f2b4fd2c62 100755
> > --- a/configure
> > +++ b/configure
> > @@ -4568,7 +4568,7 @@ set >> $logfile
> >  test -n "$valgrind" && toolchain="valgrind-memcheck"
> >
> >  enabled ossfuzz && ! echo $CFLAGS | grep -q -- "-fsanitize="  && ! echo
> $CFLAGS | grep -q -- "-fcoverage-mapping" &&{
> > -    add_cflags  -fsanitize=address,undefined
> -fsanitize-coverage=trace-pc-guard,trace-cmp -fno-omit-frame-pointer
> > +    add_cflags  -fsanitize=address,undefined
> -fsanitize-coverage=trace-pc-guard,trace-cmp -fno-omit-frame-pointer
> -fno-sanitize-recover=all
> >      add_ldflags -fsanitize=address,undefined
> -fsanitize-coverage=trace-pc-guard,trace-cmp
> >  }
> >
> > @@ -4591,7 +4591,7 @@ add_sanitizer_flags(){
> >              add_ldflags -fsanitize=thread
> >          ;;
> >          usan)
> > -            add_cflags  -fsanitize=undefined
> > +            add_cflags  -fsanitize=undefined -fno-sanitize-recover=all
>
> I agree it would be good to return a nonzero exit code on detecting
> undefined behaviour when running FATE, but this sets the flag for any
> --toolchain=*-usan configuration.  Personally, I would find it a little
> unexpected that compiling with --toolchain=*-usan results in anything
> but the default behaviour of UBSAN, and one might wish to use UBSAN
> without the flag when testing manually.  As an alternative, what about
> instead setting UBSAN_OPTIONS=halt_on_error=1 only when running the FATE
> suite or fuzzing?
>
> --
> Frank
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] configure: Use -fno-sanitize-recover

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 3973 bytes --]

On 1/17/2025 7:53 PM, Vitaly Buka via ffmpeg-devel wrote:
> My confusion here is that it looks like ffmpeg developers care about UB, I
> see from time to time large cleanups, but there are a bunch of unfixed
> reports.
> Maybe forcing no-recover by default will improve this situation?

It will not change much because the FATE clients currently running with 
gcc ubsan both manually add this extra option, but with undefined as 
argument rather than all. See 
https://fate.ffmpeg.org/report.cgi?time=20250117151351&slot=x86_64-archlinux-gcc-ubsan

Like i said, I'm not against adding this extra option, but I'm against 
adding all the exceptions scattered around the code.

> 
> 
> On Fri, Jan 17, 2025 at 11:57 AM Frank Plowman <post@frankplowman.com>
> wrote:
> 
>> On 16/01/2025 19:12, Vitaly Buka via ffmpeg-devel wrote:
>>> UBSAN by default is just prints a mesage and
>>> moves on. This hides a few UBs in fate-suite.
>>>
>>> Signed-off-by: Vitaly Buka <vitalybuka@google.com>
>>> ---
>>>   configure                     | 4 ++--
>>>   libavcodec/aacenc_pred.c      | 1 +
>>>   libavcodec/ffv1dec.c          | 1 +
>>>   libavcodec/ffv1enc_template.c | 1 +
>>>   libavcodec/get_bits.h         | 1 +
>>>   libavcodec/indeo3.c           | 2 +-
>>>   libavcodec/motion_est.c       | 1 +
>>>   libavcodec/mss2dsp.c          | 1 +
>>>   libavcodec/opus/dec.c         | 1 +
>>>   libavcodec/snow.h             | 1 +
>>>   libavcodec/svq1enc.c          | 1 +
>>>   libavfilter/vf_curves.c       | 1 +
>>>   libavfilter/vf_overlay.c      | 1 +
>>>   libavformat/mov.c             | 1 +
>>>   libswscale/input.c            | 6 ++++++
>>>   libswscale/output.c           | 4 ++++
>>>   libswscale/swscale_unscaled.c | 3 +++
>>>   17 files changed, 28 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 3a1e72e1c6..f2b4fd2c62 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -4568,7 +4568,7 @@ set >> $logfile
>>>   test -n "$valgrind" && toolchain="valgrind-memcheck"
>>>
>>>   enabled ossfuzz && ! echo $CFLAGS | grep -q -- "-fsanitize="  && ! echo
>> $CFLAGS | grep -q -- "-fcoverage-mapping" &&{
>>> -    add_cflags  -fsanitize=address,undefined
>> -fsanitize-coverage=trace-pc-guard,trace-cmp -fno-omit-frame-pointer
>>> +    add_cflags  -fsanitize=address,undefined
>> -fsanitize-coverage=trace-pc-guard,trace-cmp -fno-omit-frame-pointer
>> -fno-sanitize-recover=all
>>>       add_ldflags -fsanitize=address,undefined
>> -fsanitize-coverage=trace-pc-guard,trace-cmp
>>>   }
>>>
>>> @@ -4591,7 +4591,7 @@ add_sanitizer_flags(){
>>>               add_ldflags -fsanitize=thread
>>>           ;;
>>>           usan)
>>> -            add_cflags  -fsanitize=undefined
>>> +            add_cflags  -fsanitize=undefined -fno-sanitize-recover=all
>>
>> I agree it would be good to return a nonzero exit code on detecting
>> undefined behaviour when running FATE, but this sets the flag for any
>> --toolchain=*-usan configuration.  Personally, I would find it a little
>> unexpected that compiling with --toolchain=*-usan results in anything
>> but the default behaviour of UBSAN, and one might wish to use UBSAN
>> without the flag when testing manually.  As an alternative, what about
>> instead setting UBSAN_OPTIONS=halt_on_error=1 only when running the FATE
>> suite or fuzzing?
>>
>> --
>> Frank
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] configure: Use -fno-sanitize-recover

[Vitaly Buka via ffmpeg-devel]

On Fri, Jan 17, 2025 at 3:12 PM James Almer <jamrial@gmail.com> wrote:

> On 1/17/2025 7:53 PM, Vitaly Buka via ffmpeg-devel wrote:
> > My confusion here is that it looks like ffmpeg developers care about UB,
> I
> > see from time to time large cleanups, but there are a bunch of unfixed
> > reports.
> > Maybe forcing no-recover by default will improve this situation?
>
> It will not change much because the FATE clients currently running with
> gcc ubsan both manually add this extra option, but with undefined as
> argument rather than all. See
>
> https://fate.ffmpeg.org/report.cgi?time=20250117151351&slot=x86_64-archlinux-gcc-ubsan
>
> Like i said, I'm not against adding this extra option, but I'm against
> adding all the exceptions scattered around the code.
>
>
Sorry. Looks like I fixed my email delivery after your reply.
I can see it in the archive now:

> Adding this is probably fine, but all the exceptions below to ignore
issues are not ok.

Does this mean we need to fix those issues first?
Is it acceptable to make FATE stop passing with UBSAN?

Most of them are relatively straight forward, but I failed to quick-guess
some, like "bounds" one.



> >
> >
> > On Fri, Jan 17, 2025 at 11:57 AM Frank Plowman <post@frankplowman.com>
> > wrote:
> >
> >> On 16/01/2025 19:12, Vitaly Buka via ffmpeg-devel wrote:
> >>> UBSAN by default is just prints a mesage and
> >>> moves on. This hides a few UBs in fate-suite.
> >>>
> >>> Signed-off-by: Vitaly Buka <vitalybuka@google.com>
> >>> ---
> >>>   configure                     | 4 ++--
> >>>   libavcodec/aacenc_pred.c      | 1 +
> >>>   libavcodec/ffv1dec.c          | 1 +
> >>>   libavcodec/ffv1enc_template.c | 1 +
> >>>   libavcodec/get_bits.h         | 1 +
> >>>   libavcodec/indeo3.c           | 2 +-
> >>>   libavcodec/motion_est.c       | 1 +
> >>>   libavcodec/mss2dsp.c          | 1 +
> >>>   libavcodec/opus/dec.c         | 1 +
> >>>   libavcodec/snow.h             | 1 +
> >>>   libavcodec/svq1enc.c          | 1 +
> >>>   libavfilter/vf_curves.c       | 1 +
> >>>   libavfilter/vf_overlay.c      | 1 +
> >>>   libavformat/mov.c             | 1 +
> >>>   libswscale/input.c            | 6 ++++++
> >>>   libswscale/output.c           | 4 ++++
> >>>   libswscale/swscale_unscaled.c | 3 +++
> >>>   17 files changed, 28 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/configure b/configure
> >>> index 3a1e72e1c6..f2b4fd2c62 100755
> >>> --- a/configure
> >>> +++ b/configure
> >>> @@ -4568,7 +4568,7 @@ set >> $logfile
> >>>   test -n "$valgrind" && toolchain="valgrind-memcheck"
> >>>
> >>>   enabled ossfuzz && ! echo $CFLAGS | grep -q -- "-fsanitize="  && !
> echo
> >> $CFLAGS | grep -q -- "-fcoverage-mapping" &&{
> >>> -    add_cflags  -fsanitize=address,undefined
> >> -fsanitize-coverage=trace-pc-guard,trace-cmp -fno-omit-frame-pointer
> >>> +    add_cflags  -fsanitize=address,undefined
> >> -fsanitize-coverage=trace-pc-guard,trace-cmp -fno-omit-frame-pointer
> >> -fno-sanitize-recover=all
> >>>       add_ldflags -fsanitize=address,undefined
> >> -fsanitize-coverage=trace-pc-guard,trace-cmp
> >>>   }
> >>>
> >>> @@ -4591,7 +4591,7 @@ add_sanitizer_flags(){
> >>>               add_ldflags -fsanitize=thread
> >>>           ;;
> >>>           usan)
> >>> -            add_cflags  -fsanitize=undefined
> >>> +            add_cflags  -fsanitize=undefined -fno-sanitize-recover=all
> >>
> >> I agree it would be good to return a nonzero exit code on detecting
> >> undefined behaviour when running FATE, but this sets the flag for any
> >> --toolchain=*-usan configuration.  Personally, I would find it a little
> >> unexpected that compiling with --toolchain=*-usan results in anything
> >> but the default behaviour of UBSAN, and one might wish to use UBSAN
> >> without the flag when testing manually.  As an alternative, what about
> >> instead setting UBSAN_OPTIONS=halt_on_error=1 only when running the FATE
> >> suite or fuzzing?
> >>
> >> --
> >> Frank
> >>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >>
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] configure: Use -fno-sanitize-recover

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 5554 bytes --]

On 1/17/2025 8:27 PM, Vitaly Buka via ffmpeg-devel wrote:
> On Fri, Jan 17, 2025 at 3:12 PM James Almer <jamrial@gmail.com> wrote:
> 
>> On 1/17/2025 7:53 PM, Vitaly Buka via ffmpeg-devel wrote:
>>> My confusion here is that it looks like ffmpeg developers care about UB,
>> I
>>> see from time to time large cleanups, but there are a bunch of unfixed
>>> reports.
>>> Maybe forcing no-recover by default will improve this situation?
>>
>> It will not change much because the FATE clients currently running with
>> gcc ubsan both manually add this extra option, but with undefined as
>> argument rather than all. See
>>
>> https://fate.ffmpeg.org/report.cgi?time=20250117151351&slot=x86_64-archlinux-gcc-ubsan
>>
>> Like i said, I'm not against adding this extra option, but I'm against
>> adding all the exceptions scattered around the code.
>>
>>
> Sorry. Looks like I fixed my email delivery after your reply.
> I can see it in the archive now:
> 
>> Adding this is probably fine, but all the exceptions below to ignore
> issues are not ok.
> 
> Does this mean we need to fix those issues first?

Ideally.

> Is it acceptable to make FATE stop passing with UBSAN?

As you can see, there are three currently failing tests if you enable 
"-fno-sanitize-recover=undefined", so it's not technically passing as is 
either.

> 
> Most of them are relatively straight forward, but I failed to quick-guess
> some, like "bounds" one.

Patches for any of them are welcome.

> 
> 
> 
>>>
>>>
>>> On Fri, Jan 17, 2025 at 11:57 AM Frank Plowman <post@frankplowman.com>
>>> wrote:
>>>
>>>> On 16/01/2025 19:12, Vitaly Buka via ffmpeg-devel wrote:
>>>>> UBSAN by default is just prints a mesage and
>>>>> moves on. This hides a few UBs in fate-suite.
>>>>>
>>>>> Signed-off-by: Vitaly Buka <vitalybuka@google.com>
>>>>> ---
>>>>>    configure                     | 4 ++--
>>>>>    libavcodec/aacenc_pred.c      | 1 +
>>>>>    libavcodec/ffv1dec.c          | 1 +
>>>>>    libavcodec/ffv1enc_template.c | 1 +
>>>>>    libavcodec/get_bits.h         | 1 +
>>>>>    libavcodec/indeo3.c           | 2 +-
>>>>>    libavcodec/motion_est.c       | 1 +
>>>>>    libavcodec/mss2dsp.c          | 1 +
>>>>>    libavcodec/opus/dec.c         | 1 +
>>>>>    libavcodec/snow.h             | 1 +
>>>>>    libavcodec/svq1enc.c          | 1 +
>>>>>    libavfilter/vf_curves.c       | 1 +
>>>>>    libavfilter/vf_overlay.c      | 1 +
>>>>>    libavformat/mov.c             | 1 +
>>>>>    libswscale/input.c            | 6 ++++++
>>>>>    libswscale/output.c           | 4 ++++
>>>>>    libswscale/swscale_unscaled.c | 3 +++
>>>>>    17 files changed, 28 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/configure b/configure
>>>>> index 3a1e72e1c6..f2b4fd2c62 100755
>>>>> --- a/configure
>>>>> +++ b/configure
>>>>> @@ -4568,7 +4568,7 @@ set >> $logfile
>>>>>    test -n "$valgrind" && toolchain="valgrind-memcheck"
>>>>>
>>>>>    enabled ossfuzz && ! echo $CFLAGS | grep -q -- "-fsanitize="  && !
>> echo
>>>> $CFLAGS | grep -q -- "-fcoverage-mapping" &&{
>>>>> -    add_cflags  -fsanitize=address,undefined
>>>> -fsanitize-coverage=trace-pc-guard,trace-cmp -fno-omit-frame-pointer
>>>>> +    add_cflags  -fsanitize=address,undefined
>>>> -fsanitize-coverage=trace-pc-guard,trace-cmp -fno-omit-frame-pointer
>>>> -fno-sanitize-recover=all
>>>>>        add_ldflags -fsanitize=address,undefined
>>>> -fsanitize-coverage=trace-pc-guard,trace-cmp
>>>>>    }
>>>>>
>>>>> @@ -4591,7 +4591,7 @@ add_sanitizer_flags(){
>>>>>                add_ldflags -fsanitize=thread
>>>>>            ;;
>>>>>            usan)
>>>>> -            add_cflags  -fsanitize=undefined
>>>>> +            add_cflags  -fsanitize=undefined -fno-sanitize-recover=all
>>>>
>>>> I agree it would be good to return a nonzero exit code on detecting
>>>> undefined behaviour when running FATE, but this sets the flag for any
>>>> --toolchain=*-usan configuration.  Personally, I would find it a little
>>>> unexpected that compiling with --toolchain=*-usan results in anything
>>>> but the default behaviour of UBSAN, and one might wish to use UBSAN
>>>> without the flag when testing manually.  As an alternative, what about
>>>> instead setting UBSAN_OPTIONS=halt_on_error=1 only when running the FATE
>>>> suite or fuzzing?
>>>>
>>>> --
>>>> Frank
>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>> To unsubscribe, visit link above, or email
>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".