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

[Mikhail Kashkarov]

^ gentle ping.


On 06/08/2018 05:54 PM, Mikhail Kashkarov wrote:
> Hello,
>
> I've updated patches for std::string sanitization and disabling CXX11
> string SSO usage for correct sanitization.
>
>   >> Â Â Â  Â  _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've checked asm code for string-inst.o and it looks like this calls are
> optimized away, but there are some light changes after patch fir .
>
>   > Right, I was wondering specifically about the <fstream>
>   > instantiations. I could be wrong but I don't think anything in
>   > <fstream> creates, destroys or modifies a std::basic_string.
>
> I was confused by reinterpret_cast's on strings in fstream.tcc, looks
> like this is not needed, you are right.
>
>   >>Â Â Â Â Â Â  // 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).
>
> Checked current version without this change, looks like now it works,
> reverted.
>
> Short summary:
> The reason for changing strings layout under sanitization is to place string
> char buffer on heap for correct aligning in 32-bit environments,
> both pre-CXX11 and CXX11 strings ABI.
>
> | Sanitize string | string type | ABI is changed? | 32-bit | 64-bit |
> |-----------------+-------------+-----------------+--------+--------|
> | FULL            | SSO-string  | yes             | +      | +      |
> |                 | COW-string  | yes             | +      | +      |
> |-----------------+-------------+-----------------+--------+--------|
> | PARTIAL         | SSO-string  | no              | -+(*)  | +      |
> |                 | COW-string  | no              | -      | +      |
> *only longs strings are sanitized for 32-bit
>
> Some functions with new define looks a bit messy without changing internal
> functions(operator=), I'm also not sure if disabling string SSO usage define
> should also affects other parts that relies on basic_string class size
> (checks
> with static_assert in exceptions/shim-facets).
>
>
> Any thoughts?
>
> On 05/29/2018 06:55 PM, Jonathan Wakely wrote:
>> On 29/05/18 18:18 +0300, Kashkarov Mikhail wrote:
>>> Jonathan Wakely <jwakely@redhat.com> writes:
>>>>> --- 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.
>> Right, I was wondering specifically about the <fstream>
>> instantiations. I could be wrong but I don't think anything in
>> <fstream> creates, destroys or modifies a std::basic_string.
>>
>>
>>
>>
>>

-- 
Best regards,
Kashkarov Mikhail
Samsung R&D Institute Russia