Subject: Re: bug#25024: Bug in Sort

From: Pádraig Brady <[email-masked]>
Date: Fri, 25 Nov 2016 22:25:26 +0000

On 25/11/16 18:50, Paul Eggert wrote:
> Pádraig Brady wrote:
>> for UBSAN we should probably build with
>> _STRING_ARCH_unaligned defined globally
>> to avoid warning for the cases we already handle.
> 
> Yes. Translating this for non-experts: the problem here is a bug in the 
> bug-finding procedure, not a bug in GNU coreutils or in Gnulib.

Sorry I was a bit terse. coreutils/gnulib should currently be compiled with
  -D_STRING_ARCH_unaligned=0 -D_STRING_INLINE_unaligned=0
when using UBSAN, to use only alignment portable code.
Methods for avoiding false UBSAN warnings automatically are discussed below...

> Recent glibc (since 2016-02-18) does not define _STRING_ARCH_unaligned, which 
> means that this code in gnulib md5.c etc. is no longer exercised on recent 
> platforms.

Oh interesting. I see details in:
https://sourceware.org/bugzilla/show_bug.cgi?id=19462
There it suggests that _STRING_ARCH_unaligned is now internal
to glibc and _STRING_INLINE_unaligned is the newer stable equivalent.
Attached patch to do this for coreutils is attached.

> So in some sense the originally-reported bug is already fixed (via an 
> unexpected glibc change), though this does mean Gnulib md5 etc. is now slower on 
> x86-64 etc., which is a performance bug on newer platforms. If we fix the 
> performance bug I suppose we'll start getting false alarms from UBSAN again.

We can explicitly avoid the UBSAN warnings with something like:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.23-80-g5760532
which might be acceptable given the few places it matters.
That's a bit of a big hammer though, defining away all of UBSAN for those routines.

Alternatively we might define the non-portable faster path away,
if we could detect we where compiling in UBSAN mode.
That's easy enough for -fsanitize=address, though it doesn't
look like there is currently a way to detect -fsanitize=undefined?
http://stackoverflow.com/q/39371798/4421

Another approach would be to support ../configure --with-asan --with-ubsan
which would define things appropriately.

cheers,
Pádraig.
From 952bd8666c7ed887d49b049a7585b56c1e93f323 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Fri, 25 Nov 2016 22:10:20 +0000
Subject: [PATCH] shred,sort: ensure faster unaligned access to rand module

glibc has changed the public define
from _STRING_ARCH_unaligned to _STRING_INLINE_unaligned as per
https://sourceware.org/bugzilla/show_bug.cgi?id=19462

* gl/lib/rand-isaac.c: Cater for both defines.
* gl/lib/randread.c: Likewise.
* src/system.h: Update commented out code.
---
 gl/lib/rand-isaac.c | 2 +-
 gl/lib/randread.c   | 2 +-
 src/system.h        | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gl/lib/rand-isaac.c b/gl/lib/rand-isaac.c
index 5ad9cae..349bf0e 100644
--- a/gl/lib/rand-isaac.c
+++ b/gl/lib/rand-isaac.c
@@ -40,7 +40,7 @@
 /* If the platform supports unaligned access,
    then don't have -fsanitize=undefined warn about it.  */
 #undef ATTRIBUTE_NO_WARN_SANITIZE_UNDEFINED
-#if !_STRING_ARCH_unaligned \
+#if !(_STRING_ARCH_unaligned || _STRING_INLINE_unaligned) \
     || __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 9)
 # define ATTRIBUTE_NO_WARN_SANITIZE_UNDEFINED /* empty */
 #else
diff --git a/gl/lib/randread.c b/gl/lib/randread.c
index ff85d56..2d211a0 100644
--- a/gl/lib/randread.c
+++ b/gl/lib/randread.c
@@ -60,7 +60,7 @@
 # define MIN(a, b) ((a) < (b) ? (a) : (b))
 #endif
 
-#if _STRING_ARCH_unaligned
+#if _STRING_ARCH_unaligned || _STRING_INLINE_unaligned
 # define ALIGNED_POINTER(ptr, type) true
 #else
 # define ALIGNED_POINTER(ptr, type) ((size_t) (ptr) % alignof (type) == 0)
diff --git a/src/system.h b/src/system.h
index e82dce4..3fa0740 100644
--- a/src/system.h
+++ b/src/system.h
@@ -515,7 +515,7 @@ is_nul (void const *buf, size_t length)
    to avoid -fsanitize=undefined warnings.
    Considering coreutils is mainly concerned with relatively
    large buffers, we'll just use the defined behavior.  */
-#if 0 && _STRING_ARCH_unaligned
+#if 0 && (_STRING_ARCH_unaligned || _STRING_INLINE_unaligned)
   unsigned long word;
 #else
   unsigned char word;
-- 
2.5.5