diff mbox series

[FFmpeg-devel,1/4] avcodec/startcode: Use AV_RN due to UBSan warning about unaligned access

Message ID 20200122145210.6898-1-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/4] avcodec/startcode: Use AV_RN due to UBSan warning about unaligned access
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 22, 2020, 2:52 p.m. UTC
when directly accessing a buffer via a pointer to uint8_t cast to a
pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED
was set, so it was ok, but UBSan nevertheless complained about unaligned
accesses. So simply use AV_RNxx to read the buffer; this also improves
readability.

This affected many FATE-tests (namely 228 of them; e.g. the cbs-h264
tests) and it has also been reported in tickets #8138 and #8485.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/startcode.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Carl Eugen Hoyos Jan. 22, 2020, 3:05 p.m. UTC | #1
Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:
>
> when directly accessing a buffer via a pointer to uint8_t cast to a
> pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED
> was set, so it was ok, but UBSan nevertheless complained about unaligned
> accesses. So simply use AV_RNxx to read the buffer; this also improves
> readability.

Is there a speed impact?

Carl Eugen
Andreas Rheinhardt Jan. 22, 2020, 3:21 p.m. UTC | #2
On Wed, Jan 22, 2020 at 4:06 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com>:
> >
> > when directly accessing a buffer via a pointer to uint8_t cast to a
> > pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED
> > was set, so it was ok, but UBSan nevertheless complained about unaligned
> > accesses. So simply use AV_RNxx to read the buffer; this also improves
> > readability.
>
> Is there a speed impact?
>
> The assembly produced by both Clang as well as GCC was completely
unchanged (for non-UBSan builds).

- Andreas
Carl Eugen Hoyos Jan. 22, 2020, 3:23 p.m. UTC | #3
Am Mi., 22. Jan. 2020 um 16:21 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:
>
> On Wed, Jan 22, 2020 at 4:06 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> > Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt
> > <andreas.rheinhardt@gmail.com>:
> > >
> > > when directly accessing a buffer via a pointer to uint8_t cast to a
> > > pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED
> > > was set, so it was ok, but UBSan nevertheless complained about unaligned
> > > accesses. So simply use AV_RNxx to read the buffer; this also improves
> > > readability.
> >
> > Is there a speed impact?
> >
> The assembly produced by both Clang as well as GCC was completely
> unchanged (for non-UBSan builds).

(on x86?)

Great!

Carl Eugen
Andreas Rheinhardt Jan. 22, 2020, 3:25 p.m. UTC | #4
On Wed, Jan 22, 2020 at 4:23 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am Mi., 22. Jan. 2020 um 16:21 Uhr schrieb Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com>:
> >
> > On Wed, Jan 22, 2020 at 4:06 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
> >
> > > Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt
> > > <andreas.rheinhardt@gmail.com>:
> > > >
> > > > when directly accessing a buffer via a pointer to uint8_t cast to a
> > > > pointer to uint64_t/uint32_t. This happened only if
> HAVE_FAST_UNALIGNED
> > > > was set, so it was ok, but UBSan nevertheless complained about
> unaligned
> > > > accesses. So simply use AV_RNxx to read the buffer; this also
> improves
> > > > readability.
> > >
> > > Is there a speed impact?
> > >
> > The assembly produced by both Clang as well as GCC was completely
> > unchanged (for non-UBSan builds).
>
> (on x86?)
>
> Yes: x86-64.

- Andreas
Andreas Rheinhardt Feb. 20, 2020, 1:40 p.m. UTC | #5
Andreas Rheinhardt:
> when directly accessing a buffer via a pointer to uint8_t cast to a
> pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED
> was set, so it was ok, but UBSan nevertheless complained about unaligned
> accesses. So simply use AV_RNxx to read the buffer; this also improves
> readability.
> 
> This affected many FATE-tests (namely 228 of them; e.g. the cbs-h264
> tests) and it has also been reported in tickets #8138 and #8485.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/startcode.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/startcode.c b/libavcodec/startcode.c
> index 9efdffe8c6..d84f326521 100644
> --- a/libavcodec/startcode.c
> +++ b/libavcodec/startcode.c
> @@ -25,6 +25,7 @@
>   * @author Michael Niedermayer <michaelni@gmx.at>
>   */
>  
> +#include "libavutil/intreadwrite.h"
>  #include "startcode.h"
>  #include "config.h"
>  
> @@ -38,14 +39,14 @@ int ff_startcode_find_candidate_c(const uint8_t *buf, int size)
>       */
>  #if HAVE_FAST_64BIT
>      while (i < size &&
> -            !((~*(const uint64_t *)(buf + i) &
> -                    (*(const uint64_t *)(buf + i) - 0x0101010101010101ULL)) &
> +            !((~AV_RN64(buf + i) &
> +                    (AV_RN64(buf + i) - 0x0101010101010101ULL)) &
>                      0x8080808080808080ULL))
>          i += 8;
>  #else
>      while (i < size &&
> -            !((~*(const uint32_t *)(buf + i) &
> -                    (*(const uint32_t *)(buf + i) - 0x01010101U)) &
> +            !((~AV_RN32(buf + i) &
> +                    (AV_RN32(buf + i) - 0x01010101U)) &
>                      0x80808080U))
>          i += 4;
>  #endif
> 
Ping for this and the other unmerged patches of this patchset.

- Andreas
Michael Niedermayer Feb. 21, 2020, 8:41 p.m. UTC | #6
On Wed, Jan 22, 2020 at 04:25:40PM +0100, Andreas Rheinhardt wrote:
> On Wed, Jan 22, 2020 at 4:23 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 
> > Am Mi., 22. Jan. 2020 um 16:21 Uhr schrieb Andreas Rheinhardt
> > <andreas.rheinhardt@gmail.com>:
> > >
> > > On Wed, Jan 22, 2020 at 4:06 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> > wrote:
> > >
> > > > Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt
> > > > <andreas.rheinhardt@gmail.com>:
> > > > >
> > > > > when directly accessing a buffer via a pointer to uint8_t cast to a
> > > > > pointer to uint64_t/uint32_t. This happened only if
> > HAVE_FAST_UNALIGNED
> > > > > was set, so it was ok, but UBSan nevertheless complained about
> > unaligned
> > > > > accesses. So simply use AV_RNxx to read the buffer; this also
> > improves
> > > > > readability.
> > > >
> > > > Is there a speed impact?
> > > >
> > > The assembly produced by both Clang as well as GCC was completely
> > > unchanged (for non-UBSan builds).
> >
> > (on x86?)
> >
> > Yes: x86-64.

are any other architectures affected or is it the same for all major ones?

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/startcode.c b/libavcodec/startcode.c
index 9efdffe8c6..d84f326521 100644
--- a/libavcodec/startcode.c
+++ b/libavcodec/startcode.c
@@ -25,6 +25,7 @@ 
  * @author Michael Niedermayer <michaelni@gmx.at>
  */
 
+#include "libavutil/intreadwrite.h"
 #include "startcode.h"
 #include "config.h"
 
@@ -38,14 +39,14 @@  int ff_startcode_find_candidate_c(const uint8_t *buf, int size)
      */
 #if HAVE_FAST_64BIT
     while (i < size &&
-            !((~*(const uint64_t *)(buf + i) &
-                    (*(const uint64_t *)(buf + i) - 0x0101010101010101ULL)) &
+            !((~AV_RN64(buf + i) &
+                    (AV_RN64(buf + i) - 0x0101010101010101ULL)) &
                     0x8080808080808080ULL))
         i += 8;
 #else
     while (i < size &&
-            !((~*(const uint32_t *)(buf + i) &
-                    (*(const uint32_t *)(buf + i) - 0x01010101U)) &
+            !((~AV_RN32(buf + i) &
+                    (AV_RN32(buf + i) - 0x01010101U)) &
                     0x80808080U))
         i += 4;
 #endif