Preemptively disable unsigned integer sanitization in 32 and 64 bit versions of __murmur2_or_cityhash

[danielaustin]

Summary

When using ubsan on an x86 target, specifically when calling find on an unordered_map, unsigned integer overflow is detected and an abort occurs. More specifically, the following snippet illustrates the behavior:

#include <unordered_map>

std::unordered_map<std::string, uint32_t> lookup_table;
...
std::unordered_map<std::string, uint32_t>::const_iterator it = lookup_table.find(item);

The find call eventually calls down to __murmur2_or_cityhash, which contains the benign unsigned integer overflow. Disabling sanitization for this function prevents the abort from occurring.

[mclow.lists]

My first reaction to this patch is "this code is doing perfectly legal things. UBSAN should not be complaining about it."

My second thought is "if we have to do this, why can't we use C++ attributes instead of __attribute?"
What happens if you use GCC (since gcc 4.9 and later support UBSAN).

[danalbert]

In D15973#322399, @mclow.lists wrote:

My first reaction to this patch is "this code is doing perfectly legal things. UBSAN should not be complaining about it."

ubsan has a switch for flagging unsigned integer overflows too. While well defined, we've found that they can cause all kinds of security issues, so for a number of projects in Android we have enabled this check for mitigation purposes. This patch just tells the compiler that we know we're relying on the wrapping behavior here, and that it shouldn't abort in this case.

I had spoken with @EricWF about this yesterday and he said that it was something he'd been meaning to do for a while.

[mclow.lists]

I would suggest a different direction:
In <__config> define a macro named something like _LIBCPP_DISABLE_ASAN_UNSINGNED_INTEGER_CHECK to be either empty or __attribute((no_sanitize("unsigned integer"))), and use that in <memory>.

That way:

• A user can define it at build time
• We can use different text for different compilers.

[danielaustin]

That seems reasonable to me. Steve, Dan, any forseeable issues there?

[danalbert]

SGTM.

[mclow.lists]

Details:

in the clang section of __config, add:

#ifndef _LIBCPP_DISABLE_ASAN_UNSINGNED_INTEGER_CHECK
#define _LIBCPP_DISABLE_ASAN_UNSINGNED_INTEGER_CHECK __attribute((no_sanitize("unsigned integer")))
#endif

at the end of __config add:

#ifndef _LIBCPP_DISABLE_ASAN_UNSINGNED_INTEGER_CHECK
#define _LIBCPP_DISABLE_ASAN_UNSINGNED_INTEGER_CHECK
#endif

and then use _LIBCPP_DISABLE_ASAN_UNSINGNED_INTEGER_CHECK in <memory>.

If that's agreeable, I can just make that change.
If @danielaustin wants to prepare a new patch, we can review that.

[mclow.lists]

Maybe spelling it _LIBCPP_DISABLE_ASAN_UNSIGNED_INTEGER_CHECK instead of _LIBCPP_DISABLE_ASAN_UNSINGNED_INTEGER_CHECK would be a better idea.

[danielaustin]

I'm good with that. Probably should be called
_LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK but otherwise I see no
problems

[danalbert]

Nit: it's a UBSAN check, not an ASAN check

[mclow.lists]

@danielaustin - Have you built libc++ with your proposed change? I'm getting compile errors building the dylib.

[danielaustin]

The original change, yes. Can I see your issues?

[mclow.lists]

Here you go:

In file included from /Sources/LLVM/llvm/projects/libcxx/src/condition_variable.cpp:14:
In file included from /Sources/LLVM/llvm/projects/libcxx/include/condition_variable:112:
/Sources/LLVM/llvm/projects/libcxx/include/memory:3198:1: error: expected unqualified-id
template <class _Size>
^
/Sources/LLVM/llvm/projects/libcxx/include/memory:3351:1: error: expected unqualified-id
template <class _Size>
^
In file included from /Sources/LLVM/llvm/projects/libcxx/src/mutex.cpp:11:
In file included from /Sources/LLVM/llvm/projects/libcxx/include/mutex:177:
In file included from /Sources/LLVM/llvm/projects/libcxx/include/functional:477:
/Sources/LLVM/llvm/projects/libcxx/include/memory:3198:1: error: expected unqualified-id
template <class _Size>
^
/Sources/LLVM/llvm/projects/libcxx/include/memory:3351:1: error: expected unqualified-id
template <class _Size>
^
2 errors generated.

if I move the __attribute after the template <class _Size> it works better; but warns about "unknown attribute" instead.

[danielaustin]

I got these in the android build as well, but not on llvm ToT (weird...) If
the __attribute line was unconditional (no ifdef's) and on the same line as
the function it was applied to, it compiled and worked as expected.

Just resync'ed will try again on llvm and see what I get

[danielaustin]

Ah, yes. clang is not the default on my workstation (used to build configs
setting custom clang path for me). It builds fine with gcc, but I see
exactly what you are seeing with clang.

[mclow.lists]

I'll wait for you to update your patch, then.

[danielaustin]

Refactored as Marshall suggested. Successfully compiled with clang & gcc.

[mclow.lists]

Other than the placement of the macro in memory, this looks good to me.

include/memory

3197

That's not where I would have expected it to go - I would have thought it should go after the parameter list. See http://clang.llvm.org/docs/AttributeReference.html#function-attributes

But if it works, that's fine.

3347

Same comment as #3197

[mclow.lists]

Moving the macro to the end of the parameter list works fine using clang.

[danielaustin]

Yep, just finished testing. Let's do that since its consistent with the
reference.

The macro is in __config

[mclow.lists]

Can you commit this, or do you need me to do so?

[danielaustin]

Never tried, probably will need you do as I see no obvious commit option
for me.

[mclow.lists]

landed as revision 257368.

[danielaustin]

Yep, I rock. Missed the right location to activate this for clang. This diff fixes that issue.

[mclow.lists]

And I think that for clang, the actual flag you want is no_sanitize("unsigned-integer-overflow")

[danielaustin]

They both have the same effect on the target I am working with. I'm ok with
changing it if that's more proper than what I have.

[mclow.lists]

On Mac OS, "unsigned integer" gives a warning.

warning: unknown sanitizer 'unsigned integer' ignored [-Wunknown-sanitizers]

landed as revision 257422

[EricWF]

Can we close this revision?

include/__config

 
 #define _LIBCPP_HAS_NO_ASAN
 
+// Allow for build-time disabling of unsigned integer sanitization
+#ifndef _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK
+#define _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK __attribute((no_sanitize("unsigned integer")))
+#endif 
+
 #endif // __clang__ || __GNUC__ || _MSC_VER || __IBMCPP__
 
 #ifndef _LIBCPP_HAS_NO_NOEXCEPT

 #define _LIBCPP_HAS_NO_ATOMIC_HEADER
 #endif
 
+#ifndef _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK
+#define _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK
+#endif 
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG

include/memory

 // murmur2
 template <class _Size>
 _Size
-__murmur2_or_cityhash<_Size, 32>::operator()(const void* __key, _Size __len)
+__murmur2_or_cityhash<_Size, 32>::operator()(const void* __key, _Size __len) _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK 
 {
     const _Size __m = 0x5bd1e995;
     const _Size __r = 24;

 // cityhash64
 template <class _Size>
 _Size
-__murmur2_or_cityhash<_Size, 64>::operator()(const void* __key, _Size __len)
+__murmur2_or_cityhash<_Size, 64>::operator()(const void* __key, _Size __len) _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK 
 {
   const char* __s = static_cast<const char*>(__key);
   if (__len <= 32) {