Re: [2/2] Add AddressSanitizer annotations to std::string.

[Kashkarov Mikhail <m dot kashkarov at partner dot samsung dot com>]

Jonathan Wakely <jwakely@redhat.com> writes:

> On 29/05/18 09:55 +0300, Mikhail Kashkarov wrote:
>>Add AddressSanitizer annotations to std::string.
>
> Thanks for the patch, this is a significant piece of work, and a very
> desirable feature.
>
> I have some questions about the design for when & how the annotations
> are enabled, and some stylistic comments about naming.
>
>
>>--- a/libstdc++-v3/doc/xml/manual/using.xml
>>+++ b/libstdc++-v3/doc/xml/manual/using.xml
>>@@ -992,6 +992,27 @@ g++ -Winvalid-pch -I. -include stdc++.h -H -g -O2 hello.cc -o test.exe
>>       </para>
>>     </listitem></varlistentry>
>> 
>>+    <varlistentry><term><code>_GLIBCXX_SANITIZE_STRING</code></term>
>>+    <listitem>
>>+      <para>
>>+       Undefined by default. When defined, <classname>std::string</classname>
>>+        operations will be annotated so that AddressSanitizer can detect
>>+        invalid accesses to the unused capacity of a
>>+        <classname>std::string</classname>. These annotations are only
>>+        enabled for
>>+        <classname>std::string&lt;T, std::allocator&lt;T&gt;&gt;</classname>
>
> This should be std::basic_string not std::string.
>

Ok.

>>+        and only when <classname>std::allocator</classname> is derived from
>>+        <xref linkend="allocator.impl"><classname>new_allocator</classname>
>>+        or <classname>malloc_allocator</classname></xref>. The annotations
>>+        must be present on all string operations or none, so this macro must
>>+        be defined to the same value for all translation units that create,
>>+        destroy or modify strings. COW-strings are aligned by 8 under annotations
>>+	(layout change) and by default this define implicitly turn on
>>+	_GLIBCXX_DISABLE_STRING_SSO_USAGE to disable small-string optimization
>>+	for correct AddressSanitizer poisoning.
>>+      </para>
>>+    </listitem></varlistentry>
>
> These required layout changes are unfortunate, and will probably limit
> the use of ASan annotations for std::string. Not everybody can
> recompile their entire application and all its libraries.
>
> For the SSO string, would it be acceptable for the annotations to only
> apply to strings using the heap? That would fail to diagnose misuses
> of small strings, but could make the annotations useful for a much
> larger audience. Although accessing unused capacity in the small
> string buffer is still undefined, it doesn't risk accessing
> un-allocated memory, or memory that belongs to an unrelated, adjacent
> allocation on the heap.
>
> The current design means that SSO is disabled even for
> std::basic_string specializations that can't benefit from ASan
> annotations because they use custom allocators.
>
>

Annotations can be applied with sso-strings, but there are some corner
cases when annotations are failed due to aligning.

Here is an example:

struct A {
  char buf1[16];
  int i;
  char c1;
  std::string s1;
  char c2;
  char buf2[16];
  long li;
  std::string s3;
  char c3;
  char buf3[16];
  std::string s2;
  void f();
};

void A::f() {
  buf1[0] = '1';
  buf2[0] = '2';
  buf3[0] = '3';
  i = 1;
  li = 12l;
};

int main() {
  A a;
  a.f();
}

bash-3.2# g++ -v -Wl,-t 2>&1 | grep sanitize

COLLECT_GCC_OPTIONS='-v' '-fsanitize-recover=address'
'-fsanitize=address' '-D' '_GLIBCXX_SANITIZE_STRING'
'-fno-omit-frame-pointer' '-fno-common' '-shared-libgcc'
'-mtune=generic' '-march=i586'

bash-3.2# g++ -O0 -g test.cc
bash-3.2# gdb a.out 
GNU gdb (GDB) 7.12.1
Reading symbols from a.out...done.
(gdb) b main
Breakpoint 1 at 0x4019bb: file test.cc, line 28.
(gdb) r
Starting program: /a.out 
Breakpoint 1, main () at test.cc:28
28   A a;
(gdb) p &a.s1
$5 = (std::__cxx11::string *) 0xffffd54c 
(gdb) p &a.s2
$6 = (std::__cxx11::string *) 0xffffd5a8
(gdb) p &a.s3
$7 = (std::__cxx11::string *) 0xffffd57c <== not 8-align

In such structs _M_local_buf can be not aligned by 8 under ASan and that
leads to failed annotations.


>>diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
>>index d37aa0f..0acf986 100644
>>--- a/libstdc++-v3/include/bits/basic_string.h
>>+++ b/libstdc++-v3/include/bits/basic_string.h
>>@@ -48,11 +48,98 @@
>> # include <string_view>
>> #endif
>> 
>>+#if _GLIBCXX_SANITIZE_STD_ALLOCATOR && _GLIBCXX_SANITIZE_STRING
>>+# define _GLIBCXX_SANITIZER_ANNOTATE_STRING 1
>>+#endif
>>+
>>+// Disable small-string optimization for annotations
>>+#if !_GLIBCXX_DISABLE_SSO_USAGE && _GLIBCXX_SANITIZE_STRING
>>+# define _GLIBCXX_DISABLE_SSO_USAGE 1
>>+#endif
>
> Should the above lines above be in <bits/c++config.h> instead of here?
>

Will move there, also will fix typo.

>>+#if _GLIBCXX_SANITIZER_ANNOTATE_STRING
>>+extern "C"  void
>>+__sanitizer_annotate_contiguous_container(const void *, const void *,
>>+					  const void *, const void *);
>>+# define _GLIBCXX_SANITIZER_ALIGN __attribute__((aligned(8)))
>>+#else
>>+# define _GLIBCXX_SANITIZER_ALIGN
>>+#endif
>> 
>> namespace std _GLIBCXX_VISIBILITY(default)
>> {
>> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> 
>>+  template <typename _CharT, typename _Alloc>
>>+    struct _asan_traits
>
> This needs to use a reserved name, e.g. _Asan_traits or __asan_traits.
>

Ok.

>>+    {
>>+      typedef __gnu_cxx::__alloc_traits<_Alloc> __alloc_traits;
>>+      typedef typename __alloc_traits::const_pointer __const_pointer;
>
> This can just be called const_pointer. That name is already reserved
> for the library.
>
>

Ok.

>>+
>>+      template<typename, typename, typename> friend class basic_stringbuf;
>>+      static void __annotate_delete(__const_pointer __beg,
>>+				    __const_pointer __mid,
>>+				    __const_pointer __end) { }
>>+
>>+      static void __annotate_new(__const_pointer __beg, __const_pointer __mid,
>>+				 __const_pointer __end) { }
>>+
>>+      static void __annotate_grow(__const_pointer __beg,
>>+				  __const_pointer __old_mid,
>>+				  __const_pointer __new_mid,
>>+				  __const_pointer __end) { }
>>+    };
>>+
>>+  // AddressSanitizer is enabled only for default allocator.
>>+#if _GLIBCXX_SANITIZER_ANNOTATE_STRING
>>+  template <typename _CharT>
>>+    struct _asan_traits<_CharT, allocator<_CharT> >
>>+    {
>>+      typedef allocator<_CharT> __allocator_char;
>>+      typedef __gnu_cxx::__alloc_traits<__allocator_char> __alloc_traits;
>
> Are these typedefs needed? It looks like you could just do:
>
>
>>+      typedef typename __alloc_traits::const_pointer __const_pointer;
>
>    typedef typename
>      __gnu_cxx::__alloc_traits<allocator_CharT>::const_pointer
>      const_pointer;
>

Ok.

>>+
>>+      static void _GLIBCXX_VISIBILITY(hidden) __annotate_delete(
>>+						__const_pointer __beg,
>>+				    		__const_pointer __mid,
>>+				    		__const_pointer __end)
>>+	{
>>+	  __annotate_contiguous_container(__beg, __end, __mid, __end);
>>+	}
>>+
>>+      static void _GLIBCXX_VISIBILITY(hidden) __annotate_new(
>>+						__const_pointer __beg,
>>+						__const_pointer __mid,
>>+				 		__const_pointer __end)
>>+	{
>>+	  __annotate_contiguous_container(__beg, __end, __end, __mid);
>>+	}
>>+
>>+      static void _GLIBCXX_VISIBILITY(hidden) __annotate_grow(
>>+						__const_pointer __beg,
>>+				  		__const_pointer __old_mid,
>>+				  		__const_pointer __new_mid,
>>+				  		__const_pointer __end)
>>+	{
>>+	  __annotate_contiguous_container(__beg, __end, __old_mid, __new_mid);
>>+	}
>>+
>>+    private:
>>+      static void _GLIBCXX_VISIBILITY(hidden) __annotate_contiguous_container(
>>+						const void *__beg,
>
> Please place the ptr-operator with the type not the variable name,
> i.e. const void* __beg.
>
>

Ok.

>>+						const void *__end,
>>+						const void *__old_mid,
>>+						const void *__new_mid)
>>+      {
>>+        if (__beg)
>>+          {
>>+            __sanitizer_annotate_contiguous_container(__beg, __end, __old_mid,
>>+                                                      __new_mid);
>>+          }
>>+      }
>>+    };
>>+#endif // _GLIBCXX_SANITIZER_ANNOTATE_STRING
>>+
>> #if _GLIBCXX_USE_CXX11_ABI
>> _GLIBCXX_BEGIN_NAMESPACE_CXX11
>>   /**
>>@@ -96,11 +183,130 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>> 							const_iterator;
>>       typedef std::reverse_iterator<const_iterator>	const_reverse_iterator;
>>       typedef std::reverse_iterator<iterator>		reverse_iterator;
>>+      typedef _asan_traits<_CharT, _Char_alloc_type> __asan_traits;
>
> If _asan_traits is renamed to __asan_traits then you'd need to
> disambiguate it here:
>
>       typedef std::__asan_traits<_CharT, _Char_alloc_type> __asan_traits;
>

Ok.

>> 
>>       ///  Value returned by various member functions when they fail.
>>       static const size_type	npos = static_cast<size_type>(-1);
>> 
>>     private:
>>+      // The annotation for size increase should happen before the actual
>>+      // increase, but if an exception is thrown after that the annotation has
>>+      // to be undone.
>>+#if _GLIBCXX_SANITIZE_STRING
>
> Should this be #if _GLIBCXX_SANITIZER_ANNOTATE_STRING instead?
> Otherwise the annotator gets used even when std::allocator doesn't use
> new or malloc, and so can't be annotated.
>
>>+      struct _GLIBCXX_VISIBILITY(hidden) __RAII_IncreaseAnnotator
>
> This name doesn't need a double underscore prefix if it starts with an
> uppercase letter, _RAII_IncreaseAnnotator is OK.
>

Ok.

>>+      {
>>+	__RAII_IncreaseAnnotator (const basic_string &__s,
>
> West coast ptr-operator again, i.e. const basic_string& __s
>
>
>>+      _GLIBCXX_VISIBILITY(hidden) const_pointer __get_beg () const
>
> These new basic_string member functions should follow our naming
> conventions, either _M_get_beg or (for static members) _S_get_beg.
>

Ok.

>>+      {
>>+	if (!_M_is_local())
>>+	  return _M_data();
>>+	return _M_local_data();
>>+      }
>>+
>>+      _GLIBCXX_VISIBILITY(hidden) const_pointer __get_mid(size_type __sz) const
>>+      {
>>+	if (!_M_is_local())
>>+	  return _M_data() + __sz + 1 /* terminator */;
>>+	else
>>+	  return _M_local_data() + __sz + 1;
>>+      }
>>+
>>+      _GLIBCXX_VISIBILITY(hidden) const_pointer __get_end() const
>>+      {
>>+	if (!_M_is_local())
>>+	  return _M_data() + _M_allocated_capacity + 1;
>>+
>>+	const_pointer __p = __get_beg() + _S_local_capacity + 1;
>>+
>>+	return __p;
>>+      }
>>+
>>+      void __annotate_new(size_type __sz) const _GLIBCXX_VISIBILITY(hidden)
>>+	{
>>+#if _GLIBCXX_DISABLE_STRING_SSO_USAGE
>>+          if (!capacity())
>>+            return;
>>+#endif
>>+	  __asan_traits::__annotate_new(__get_beg(), __get_mid(__sz),
>>+                                  __get_end());
>>+	}
>>+
>>+      void __annotate_grow(size_type __old_size, size_type __new_size) const
>>+      _GLIBCXX_VISIBILITY(hidden)
>>+	{
>>+#ifdef _GLIBCXX_DISABLE_STRING_SSO_USAGE
>>+          if (!capacity())
>>+            return;
>>+#endif
>>+          __asan_traits::__annotate_grow(__get_beg(), __get_mid(__old_size),
>>+                                   __get_mid(__new_size), __get_end());
>>+	}
>>+
>>+      basic_string<_CharT, _Traits, _Alloc> const *__annotate_delete() const
>>+      _GLIBCXX_VISIBILITY(hidden)
>>+	{
>>+#ifdef _GLIBCXX_DISABLE_STRING_SSO_USAGE
>>+          if (!capacity())
>>+            return 0;
>>+#endif
>>+	  if (_M_is_local())
>>+	    __asan_traits::__annotate_delete(__get_beg(), __get_mid(0),
>>+                                       __get_end());
>>+	  else
>>+	    __asan_traits::__annotate_delete(__get_beg(), __get_mid(size()),
>>+                                       __get_end());
>>+	  return this;
>>+	}
>>+
>>       // type used for positions in insert, erase etc.
>> #if __cplusplus < 201103L
>>       typedef iterator __const_iterator;
>>@@ -225,6 +431,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>>       {
>> 	if (!_M_is_local())
>> 	  _M_destroy(_M_allocated_capacity);
>>+        else
>>+	  __annotate_delete();
>
> Do these calls definitely optimize away completely when not
> sanitizing? Even for -O1, -Os and -Og?
>
> For std::vector annotation I used macros to add these annotations, so
> there is no change to the generated code when annotations are
> disabled. But it makes the code quite ugly.
>
>

I'll gather some results with different optimizations and come back then.

>>@@ -3201,13 +3427,48 @@ _GLIBCXX_END_NAMESPACE_CXX11
>> 	size_type		_M_length;
>> 	size_type		_M_capacity;
>> 	_Atomic_word		_M_refcount;
>>-      };
>>+      } _GLIBCXX_SANITIZER_ALIGN;
>
> Was the annotation actually failing without this increased alignment?
> Since the annotations are only enabled for std::allocator when it uses
> operator new or malloc, won't the memory always be aligned to
> std::max_align_t anyway? So although a _Rep created on the stack won't
> currently be guaranteed to be aligned to 8 bytes, in practice the _Rep
> created on the heap should be.
>
> Or is the problem that the block of characters following the _Rep
> needs to be aligned to 8 bytes, rather than the _Rep object itself?
>
>

The problem is with 32-bit environment, _Rep is aligned by 8, but the
beginning of the string buffer is (*this + 1) and sizeof(*this) may be
not aligned by 8 (in my case it was 12). And ASan annotation fucntion
requires that the char buffer array should be 8-aligned and that leads
to fail.

The problem is that cow-strings are still used in exceptions and they
may occur with c++11 ABI strings, so the align attribute is here.
Better solution would be if ASan will align such structs, I may look
into sanitizer runtime more precisely. 

>>@@ -1094,6 +1147,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>       // have 3.4.x compiled code with _S_create callers inlined
>>       // calling 4.0.x+ _S_create.
>>       __p->_M_set_sharable();
>>+#if _GLIBCXX_SANITIZER_ANNOTATE_STRING
>>+      __p->_M_length = 0;
>>+#endif
>
> Why is this needed? I think a comment explaining it would help (like
> the one above explaining why calling _M_set_sharable() is needed).
>
>

In general we need to have actual length of allocated buffer to perform
annotation right after it (length is used to get [beg,mid) - "good"
buffer); for example, in _S_construct(size_type, _CharT, const _Alloc&)
annotation performs before _M_set_length.
I'll look into this one more closely, thanks.

>>--- a/libstdc++-v3/include/bits/fstream.tcc
>>+++ b/libstdc++-v3/include/bits/fstream.tcc
>>@@ -1081,6 +1081,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> 
>>   // Inhibit implicit instantiations for required instantiations,
>>   // which are defined via explicit instantiations elsewhere.
>>+#if !_GLIBCXX_SANITIZE_STRING
>> #if _GLIBCXX_EXTERN_TEMPLATE
>>   extern template class basic_filebuf<char>;
>>   extern template class basic_ifstream<char>;
>>@@ -1094,6 +1095,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>   extern template class basic_fstream<wchar_t>;
>> #endif
>> #endif
>>+#endif // !_GLIBCXX_SANITIZE_STRING
>
> Why do we need to disable these explicit instantiation declarations?
> Are they affected by the std::string layout changes? Is that just
> because of the constructors taking std::string, or something else?

Libstdc++ build is not sanitized, so macroses that requires
AddressSanitizer support will not applied and these templates will be
instantate without support for ASan annotations.


Thank you for reviewing time, I'll come back with updated patch.