This is the mail archive of the
libstdc++@gcc.gnu.org
mailing list for the libstdc++ project.
Re: [2/2] Add AddressSanitizer annotations to std::string.
- From: Jonathan Wakely <jwakely at redhat dot com>
- To: Mikhail Kashkarov <m dot kashkarov at partner dot samsung dot com>
- Cc: "libstdc++ at gcc dot gnu dot org" <libstdc++ at gcc dot gnu dot org>, Vyacheslav Barinov <v dot barinov at samsung dot com>, Ivan Baravy <i dot baravy at samsung dot com>
- Date: Tue, 29 May 2018 15:17:42 +0100
- Subject: Re: [2/2] Add AddressSanitizer annotations to std::string.
- References: <c8b18d2a-d5c7-e3a2-53be-30b39eadeb49@partner.samsung.com> <0d8f122d-1376-5f25-4496-0d957eebdd7b@partner.samsung.com> <CGME20180529065519eucas1p1d2e1f33524940cdc783e059622769951@eucas1p1.samsung.com> <20180529065519eucas1p1d2e1f33524940cdc783e059622769951~zCx1IxDMB0595505955eucas1p1h@eucas1p1.samsung.com>
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<T, std::allocator<T>></classname>
This should be std::basic_string not std::string.
+ 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.
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?
+#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.
+ {
+ 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.
+
+ 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;
+
+ 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.
+ 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;
/// 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.
+ {
+ __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.
+ {
+ 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.
@@ -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?
@@ -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).
--- 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?