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

[James Almer]

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 at 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 at 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 at 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 at ffmpeg.org
>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>> To unsubscribe, visit link above, or email
>>>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250117/f8e4afe4/attachment.sig>