load of misaligned address in libavcodec/startcode.c

[Suhwan]

Summary of the bug:
There're two UBSAN errors, which are load of misaligned address for type 'const uint64_t' (aka 'const unsigned long'), which requires 8 byte alignment

libavcodec/startcode.c:41:17: runtime error: load of misaligned address 0x619000000a81 for type 'const uint64_t' (aka 'const unsigned long'), which requires 8 byte alignment
0x619000000a81: note: pointer points here
 00 80 2b  68 65 61 64 09 31 2e 31  3b 0a 61 63 63 65 73 73  3b 0a 73 79 6d 62 6f 6c  73 3b 0a 6c 6f
              ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/startcode.c:41:17 in 
libavcodec/startcode.c:42:22: runtime error: load of misaligned address 0x619000000a81 for type 'const uint64_t' (aka 'const unsigned long'), which requires 8 byte alignment
0x619000000a81: note: pointer points here
 00 80 2b  68 65 61 64 09 31 2e 31  3b 0a 61 63 63 65 73 73  3b 0a 73 79 6d 62 6f 6c  73 3b 0a 6c 6f
              ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/startcode.c:42:22 in 

How to reproduce:

% ./ffmpeg_g -t 2 -y -i samples/h264/station.1080p.h264 -loglevel 99 -target dvd -map 0 -disposition: v:75 wmv2 -vframes 77 -r 108 -ab 868k -b:v 251k output/tmp.webm_dash_manifest
ffmpeg version N-94887-ge55018ee11 (git master)
built on ... ubuntu 18.04 with clang-6 and UBSAN option.

[mkver]

The code in question is inside a #ifdef HAVE_FAST_UNALIGNED. Therefore your error is not a real error; you should ​disable instrumentation for this function and other functions that use HAVE_FAST_UNALIGNED (e.g. libavcodec/h2645_parse.c contains such a function).

[trem]

I also came across this UBsan complaint and beg to differ that it's a non-issue:

Per C11 (n1570) 6.3.2.3 p7:

A pointer to an object type may be converted to a pointer to a different object type.
If the resulting pointer is not correctly aligned [...] for the referenced type, the
behavior is undefined.

It does not make a difference if that UB is behind an HAVE_FAST_UNALIGNED define or not. Though it may very well be used to indicate if it's performance-wise a good idea to do those loads, as FFmpeg intents.

So we agree that this load is *possible* when that code path is hit, but the standard still says that it is UB to do through a cast. But indirection through a memcpy is a cheap way out of this dilemma. I hacked it up here:

​https://godbolt.org/z/b7M4fbrro

Please compare it to the original version here:

​https://godbolt.org/z/G1h5hfqfc

And see the assembly is identical, yet no UB is involved. I am willing to submit a patch (obeying coding / formatting standards) if there is consensus that this is an issue that should be resolved.

[Balling]

UBSAN knows better.

[mkver]

Fixed in 30e1f5ec77530fad053730a2ffd10b8286420899.