This weekend, I got an email from Nuno Lopes informing me that UBSAN now paid attention to attribute(nonnull), and he was having some problems with it going off when using libc++.
I did some investigation, and found that he was exactly right - there were places (deep inside the vector code, for example) which called std::memcpy(null, null, 0) - which is definitely UB.
In an ideal world, our C library would define ::memcpy with the correct annotations, and libc++ would import that into namespace std, and we’d be golden.
But we don’t have a C library - we use whatever is provided by the system we’re running on, so that’s not really an option.
For my testing, I changed libc++'s header:
-using ::memcpy;
+inline _LIBCPP_INLINE_VISIBILITY
+void* memcpy(void* __s1, const void* __s2, size_t __n) attribute((nonnull(1, 2)))
+{ return ::memcpy(__s1, __s2, __n); }
(similarly for memmove and memcmp), and I found several cases of simple code that now UBSAN fires off on:
such as: std::vector v; v.push_back(1);
and : int *p = NULL; std::copy(p,p,p);
This seems fairly useful to me.
I would like to hear other people’s opinions about:
-
Is adding this kind of UB detection something that people want in libc++?
-
What do people think about wrapping the C library functions to enable UBSAN to catch them (this is a separate Q from the first Q, because I can see putting these kind of parameter checks into functions that have no counterpart in the C library). Sadly, this would NOT affect calls to ::memcpy (for example), just std::memcpy.
-
Is that the best way to annotate the declarations? Is there a more portable, standard way to do this (things that start with double underscores worry me). In any case, I would probably wrap this in a macro to support compilers that don’t understand whatever mechanism we end up using.
Thanks
– Marshall
