Re: [PATCH v2 1/3] libsepol: ignore UBSAN false-positives

[Nicolas Iooss <nicolas.iooss@m4x.org>]

On Thu, Jul 1, 2021 at 8:06 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Unsigned integer overflow is well-defined and not undefined behavior.
> But it is still useful to enable undefined behavior sanitizer checks on
> unsigned arithmetic to detect possible issues on counters or variables
> with similar purpose.
>
> Annotate functions, in which unsigned overflows are expected to happen,
> with the respective Clang function attribute[1].
> GCC does not support sanitizing unsigned integer arithmetic[2].
>
>     avtab.c:76:2: runtime error: unsigned integer overflow: 6 * 3432918353 cannot be represented in type 'unsigned int'
>     policydb.c:795:42: runtime error: unsigned integer overflow: 8160943042179512010 * 11 cannot be represented in type 'unsigned long'
>     symtab.c:25:12: runtime error: left shift of 1766601759 by 4 places cannot be represented in type 'unsigned int'
>
> [1]: https://clang.llvm.org/docs/AttributeReference.html#no-sanitize
> [2]: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Thanks,
Nicolas
> ---
> v2:
>   - use a common macro as suggested by Ondrej Mosnacek
>   - mention that GCC does not support unsigned integer sanitation
>
>  libsepol/src/avtab.c    |  1 +
>  libsepol/src/policydb.c |  1 +
>  libsepol/src/private.h  | 11 +++++++++++
>  libsepol/src/symtab.c   |  4 ++++
>  4 files changed, 17 insertions(+)
>
> diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
> index 88e9d510..37e8af07 100644
> --- a/libsepol/src/avtab.c
> +++ b/libsepol/src/avtab.c
> @@ -52,6 +52,7 @@
>  /* Based on MurmurHash3, written by Austin Appleby and placed in the
>   * public domain.
>   */
> +ignore_unsigned_overflow_
>  static inline int avtab_hash(struct avtab_key *keyp, uint32_t mask)
>  {
>         static const uint32_t c1 = 0xcc9e2d51;
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index ef2217c2..0721c81e 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -789,6 +789,7 @@ static int roles_init(policydb_t * p)
>         goto out;
>  }
>
> +ignore_unsigned_overflow_
>  static inline unsigned long
>  partial_name_hash(unsigned long c, unsigned long prevhash)
>  {
> diff --git a/libsepol/src/private.h b/libsepol/src/private.h
> index 72f21262..e54cefdb 100644
> --- a/libsepol/src/private.h
> +++ b/libsepol/src/private.h
> @@ -47,6 +47,17 @@
>  #define is_saturated(x) (x == (typeof(x))-1)
>  #define zero_or_saturated(x) ((x == 0) || is_saturated(x))
>
> +/* Use to ignore intentional unsigned under- and overflows while running under UBSAN. */
> +#if defined(__clang__) && defined(__clang_major__) && (__clang_major__ >= 4)
> +#if (__clang_major__ >= 12)
> +#define ignore_unsigned_overflow_        __attribute__((no_sanitize("unsigned-integer-overflow", "unsigned-shift-base")))
> +#else
> +#define ignore_unsigned_overflow_        __attribute__((no_sanitize("unsigned-integer-overflow")))
> +#endif
> +#else
> +#define ignore_unsigned_overflow_
> +#endif
> +
>  /* Policy compatibility information. */
>  struct policydb_compat_info {
>         unsigned int type;
> diff --git a/libsepol/src/symtab.c b/libsepol/src/symtab.c
> index 9a417ca2..a6061851 100644
> --- a/libsepol/src/symtab.c
> +++ b/libsepol/src/symtab.c
> @@ -8,9 +8,13 @@
>   */
>
>  #include <string.h>
> +
> +#include "private.h"
> +
>  #include <sepol/policydb/hashtab.h>
>  #include <sepol/policydb/symtab.h>
>
> +ignore_unsigned_overflow_
>  static unsigned int symhash(hashtab_t h, const_hashtab_key_t key)
>  {
>         const char *p, *keyp;
> --
> 2.32.0
>