ASan should detect writing to a basic_string's reserved but uninitialized memory

[davidmrdavid]

Fixes #5251

Problem:

As described in the linked bug, the following program should throw under /fsanitize=address:

#include <vector>
#include <string>

int main()
{
    // This crashes (expectedly)
    //std::vector<int> vec;
    //vec.reserve(100);
    //vec.data()[50] = 1;

    // This does not crash (it should crash, like `vector`)
    std::basic_string<char> myString;
    myString.reserve(100);
    char* data = &myString[0];
    data[50] = 'A';
}

But it does not. This PR aims to fix that.

Approach:

The issue resides in xstring's reserve implementation, inlined below:

    constexpr void reserve(_CRT_GUARDOVERFLOW const size_type _Newcap) {
        // [... removed stuff]
        const size_type _Old_size = _Mypair._Myval2._Mysize;
        _Reallocate_grow_by(_Newcap - _Old_size,
            [](_Elem* const _New_ptr, const _Elem* const _Old_ptr, const size_type _Old_size)
                _STATIC_CALL_OPERATOR { _Traits::copy(_New_ptr, _Old_ptr, _Old_size + 1); });

        _Mypair._Myval2._Mysize = _Old_size;
        // [... removed stuff]

The call to _Reallocate_grow_by calls the ASan annotation machinery under the assumption that the size (i.e the initialized memory) of the string equals it's capacity (i.e the allocated memory). You can see that in the following selected snippets of _Reallocate_grow_by:

    template <class _Fty, class... _ArgTys>
    _CONSTEXPR20 basic_string& _Reallocate_grow_by(const size_type _Size_increase, _Fty _Fn, _ArgTys... _Args) {
        // reallocate to increase size by _Size_increase elements, new buffer prepared by
        // _Fn(_New_ptr, _Old_ptr, _Old_size, _Args...)
        
        // [... removed stuff]
        const size_type _New_size     = _Old_size + _Size_increase; // << [added: assumes new size is equal to capacity]
        const size_type _Old_capacity = _My_data._Myres;
        size_type _New_capacity       = _Calculate_growth(_New_size);
        auto& _Al                     = _Getal();
        const pointer _New_ptr        = _Allocate_for_capacity(_Al, _New_capacity); // throws

        // [... removed stuff]
        _ASAN_STRING_REMOVE(*this);
        _My_data._Mysize      = _New_size;
        _My_data._Myres       = _New_capacity;
        // [... removed stuff]

        _ASAN_STRING_CREATE(*this); // [added: adjusts ASan with wrong assumption]
        return *this;
    }

So the tactical fix is simple: To modify the ASan annotations on reserve after the call to _Reallocate_grow_by. This PR does just that.

Investigation notes on _Reallocate_grow_by:

This bug made me suspicious that maybe there were other latent ASan annotations bugs, so I did a quick search over the utilization of _Reallocate_grow_by in case there were other cases that seemed wrong. Spoilers: I found no such other cases.

Here's the callers of _Reallocate_grow_by, which should make it clear that it's implementation is correct in most cases.:

• append: the aforementioned assumption makes sense here, under reallocation.
• insert: the size = capacity assumption makes sense here, under reallocation, as well.

The following two cases I'm less certain of, but seem ok too:

• resize_and_overwrite: seems right as I don't see any post-operation size adjustements.
• replace: would appreciate a second eye on this one.

And of course, the last one is the known buggy reserve case.

[StephanTLavavej]

Thanks, this is great! I pushed minor nitpicks. 😻

I also updated the PR title (which will become the commit title) because "throw" implies throwing an exception, which isn't what ASan does.

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks for this significant correctness fix! All shall love Address Sanitizer and despair! 🧝‍♀️ 💍 😻