Re: [PATCH, updated] Vtable pointer verification, runtime library changes (patch 3 of 3)

[Caroline Tice <cmtice at google dot com>]

On Mon, Feb 25, 2013 at 1:14 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 25 February 2013 19:52, Caroline Tice wrote:
>> I got too excited about being done and forgot to attach the patch. :-(
>>  Sorry. Here it is.
>
> Some comments follow, mostly from reading the comments to understand
> what this patch does, it's a really interesting feature!
>
> The generated files (configure, */Makefile.in) don't need to be in the
> patch and their ChangeLog entry can be simply "Regenerated"
>
> Was configure regenerated of modified by hand? When regenerating it
> with Autoconf 2.64 I get a different output.
>

I modified the configure file by hand.  I didn't realize it was
auto-generated.  :-(

> I also get this warning when regenerating the configury bits:
> src/Makefile.am:80: variable `libvtv___la_LIBADD' is defined but no program or
> src/Makefile.am:80: library has `libvtv___la' as canonical name (possible typo)


I was trying to define them as empty (this was in the else clause,
where "--enable-vtable-verify=yes" was not specified when doing the
original
configure.  I should probably remove libvtv___la_LIBADD from the else
clause altogether.
>
> The copyright dates should be updated to 2012-2013.
>
Ok.

> vtv_add_to_log in libstdc++-v3/libsupc++/vtv_utils.cc uses va_start
> but the matching call to va_end is missing.
>

Ok.

> Am I right in thinking there's no danger of namespace pollution from
> vtv_map.h etc. because those headers will never be included unless
> explicitly requested by users?

That should be correct.

>
> I'm not sure what the rules are regarding flexible array members in
> C++ (as it's an extension) but it looks like insert_only_hash_map is a
> non-POD (formally, it has non-trivial initialization) but no
> constructor or destructor runs for it, so formally the object's
> lifetime never begins or ends, it is just a block of memory that gets
> allocated, some bytes are set, then the memory is deallocated again.
>

I will go back and look at this again.  It's been a while since I
looked at that code.

> The comments in libstdc++-v3/libsupc++/vtv_init.cc have a typo:
>
> +/* This file contains all the definitions that go into the libvtv_init
> +   library, which is part of the vtable verification feature.  This
> +   library should contain exactly two functionsa (__VLTunprotect and
> s/functionsa/functions/
>

I'll fix that.

> And in the comments in libstdc++-v3/libsupc++/vtv_rts.cc
>
> +  The actual set of valid vtable pointers for a virtual class,
> Should that be "polymorphic class" instead of "virtual class"?
>

Ok.

> Further on there are a few typos:
>
> +   pointters for the class, so we wrote our own hashtable-based symbol
> s/pointters/pointers/
>
> +   libvtv_init.so is built from vtv_init.cc.  It is designed to hel[p
> s/hel\[p/help/
>
>
> +   __VLTVerifyVtablePoitner) with stub functions that do nothing.  If
> s/Poitner/Pointer/
>
> +   initialize any of these statics with a runtime call (for ex:
> +   sysconf.
> (Unclosed parenthesis)
>
> +   the secttion offset and size, in conjunction with the data in INFO
> s/secttion/section/
>
> +      /* TODO: Meed to revisit this code for dlopen. It most probably
> +         is not unlocking the protected vtable vars after for a load
>
> s/Meed/Need/
> s/after for a load/after a load/
>
> I see a few TODO comments in the code, I assume the plan is to address
> them eventually as time permits, rather than this being a code-drop
> that becomes abandonware :-)

Yes.  They are things that we are planning on addressing but did not
seem worth holding up the patch for.