Re: [patch][google/integration] Add lightweight checks to vector::begin et. al. (issue4973065)

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

On 6 September 2011 21:19, Paul Pluzhnikov wrote:
> On Tue, Sep 6, 2011 at 12:44 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>> On 6 September 2011 20:23, Jonathan Wakely wrote:
>>>
>>> What's a dangling vector anyway? ?One that has been moved from?
>>
>> Apparently not, since a moved-from vector would pass __valid() (as
>> indeed it should)
>>
>> So I'm quite curious what bugs this catches.
>
> The garden variety "use after free":
>
> ?vector<> *v = new vector<>;
> ...
> ?delete v;
> ...
>
> ?for (it = v->begin(); it != v->end(); ++it) ?// Oops!

Eurgh, the occurrence of "delete" in anything except a destructor is a
code smell that should have led someone to find those bugs anyway!
Obviously the code above is a trivial example, but it's unforgivable
to write that - patching vector to catch it is definitely solving the
wrong problem in the wrong place!

>> The existing debug mode
>> catches some fairly subtle cases of user error such as comparing
>> iterators from different containers, or dereferencing past-the-end
>> iterators. ?This patch seems to catch user errors related to ... what?
>> ?Heap corruption? ?Using a vector after its destructor runs? ?Those
>> are pretty serious, un-subtle errors and not specific to vector, so
>> why add code to vector (code which isn't 100% reliable if it only
>> catches corruption after the memory is reused and new data written to
>> it)
>
> It is 100% percent reliable for us, due to use of a debugging allocator
> which scribbles on all free()d memory.

Aha.

> But yes, that's one more reason why this patch is not really good for
> upstream.
>
>> to detect more general problems that can happen to any type?
>
> We can't (easily) catch the general problem. This patch allows us to easily
> catch this particular instance of it.

Sure, but so does adding "assert(this);" at the top of every member
function to check you don't call through a null pointer, but that
doesn't make it a good idea :-)

I'm not 100% against a lighter weight debug mode, but it would have to
offer some pretty convincing benefits and I don't see this patch being
widely useful enough to warrant a separate mode.  Especially as the
existing debug mode is *much* faster than it used to be for some
workloads.


>> The fact the patch did catch real bugs doesn't mean it's a good idea,
>> as you say, those bugs probably could have been caught in other ways.
>
> Sure -- we have other ways to catch the these bugs. They are not very
> practical at the moment due to their runtime overhead.
>
> As for your other suggestion: enabling _GLIBCXX_DEBUG just for vector,
> that didn't occur to me and is something I'd like to explore.

It's something I do quite often, so that specific types can be
switched to use debug mode during some builds, or for some unit tests,
without having to turn on the extra checks for every use of vector (or
other std containers) in the entire app.

Also, if your code uses a typedef like:

typedef v::vector<Data> vector_type;

and you consistently use that typedef (rather than referring to
std::vector<Data> directly) then it can be relatively painless to turn
debug mode on and off for specific pieces of code.  Even the
unconventional-looking "v::vector" (or "my_debug_switch_123::vector")
only gets mentioned in one place.