Re: detecting "container overflow" bugs in std::vector

[Konstantin Serebryany <konstantin dot s dot serebryany at gmail dot com>]

On Mon, May 26, 2014 at 6:12 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 26/05/14 17:40 +0400, Konstantin Serebryany wrote:
>>
>> Would you consider a patch similar to [4] for libstdc++ trunk?
>> If yes, any comments on the patch?
>
>
> + // When sanitizer annotataions are off, avoid bazillion of no-op
>                    ^^^^^^^^^^^^
>
> I'd rather see the member functions use our consistent naming
> conventions.
>
> The function parameters in the __is_same_allocator<T,U> case should be
> unnamed, to avoid warnings with -Wsystem-headers.
>
> The __old_size variable should be marked __attribute((unused)) too, or
> just get rid of it and pass size() - 1 to the annotate function.
>
>
>> The current patch has a (minor) problem that affects only code with
>> exceptions [5].
>
>
> So "only" most C++ code then. That's not minor.
> Not everyone turns off exceptions like Google.

The only case where the problem appears is a CTOR throwing an exception
while being called from push_back. And this will only cause false negatives.
So, this is not major.
Anyway, surely we are going to fix this (with RAII).
Just wanted to make sure there is enough interest in the libstdc++
patch before doing this.
>
>
>> If the libstdc++ team is generally ok with the idea I will work on the
>> updated patch
>> and send it for review.
>
>
> It does look useful but I'm concerned about a proliferation of
> container checks, we already have the libstdc++ Debug Mode
> and I'd
> like to see some of the lightweight checks from the Google branch
> added to trunk too.
Me too, but these checks are mostly orthogonal to the proposed annotations.

>
> The exception problem is a showstopper, but should be fixable with
> some RAII.
>

> I see that the patch on the Google branch removes some of the
> __google_stl_debug_vector checks -- are they considered no longer
> necessary/useful with asan?

This was some unrelated change. Paul?

> Aren't they still much cheaper than asan
> instrumentation?

Of course, they are much cheaper than asan. But they do not cover the case
that motivated the container overflow annotations (when the contents
of vector are accessed via
vector<T>::data())

--kcc