Re: [PATCH/Merge Request] Vtable Verification feature.

["Joseph S. Myers" <joseph at codesourcery dot com>]

Looking at the patch as committed, there seems to be some confusion about 
the nature of the --enable-vtable-verify configure option.

It's documented only for libstdc++.  Now, I still consider the existence 
of separate documentation for libstdc++ configure options to be 
unfortunate - I think all configure options for GCC should be documented 
in one place - but that's a separate matter.  Although only documented for 
libstdc++, it actually appears to be used in the libgcc and libvtv 
configure scripts.

Given that it has effects on more than just libstdc++, it needs 
documenting in gcc/doc/install.texi, the main documentation of configure 
options for GCC.  Then there's the question of what the semantics should 
be.  My presumption is that the feature should be available for all GCC 
builds, at least by default on platforms where it can work, and the only 
thing needing a configure option should be whether the checks are enabled 
for libstdc++ itself (and ideally that would work by multilibbing 
libstdc++ rather than needing separate GCC builds to get libstdc++ with 
and without the checks).  Thus if the platform supports the feature, all 
relevant libgcc files should be built, and anything for libstdc++ needed 
for user code to use the feature should be built - the only thing not 
enabled by default would be checks for libstdc++'s classes' own vtables.  
(And unless there are difficulties in building the libgcc files on some 
systems, they could be built unconditionally, whether or not any other 
support needed for libvtv is present.  Actually, it looks like they may 
depend on an ELF target, but not on anything more.)

Could you confirm that the libstdc++ ABI is not affected by the configure 
option - that the same symbols, at the same symbol versions, with the same 
semantics, are exported from libstdc++.so for builds with and without the 
feature enabled?

The file cp/vtable-class-hierarchy.c includes "tm.h".  Includes of tm.h 
from front ends are discouraged, and should have comments on them listing 
what target macros are used by the file in question (and so need to be 
converted to hooks before the include can be removed).  Could you add such 
a comment to the #include (or if it's redundant, remove all redundant 
#includes from that file)?

You have a

+#define MAX_SET_SIZE 5000

which superficially looks like an arbitrary limit.  Could you add a 
comment explaining why no input files, not matter how extreme, could ever 
exceed the limit of 5000?  You have a couple of gcc_asserts regarding this 
limit, and an on-stack array for which it's at least not immediately 
obvious that all accesses are checked to ensure that a buffer overrun is 
impossible.

If you have an arbitrary limit, and some input *can* exceed it (so 
triggering the gcc_asserts or buffer overrun), the right fix is of course 
to remove it, probably using vec.h to produce a dynamically growing array 
instead of hardcoding a size at all.  But failing that, exceeding the 
limit must result in a sorry () call, not an assertion failure or buffer 
overrun, neither of which is acceptable behavior for any compiler input 
whatever.

Other people have previously commented about the logs *generated by the 
compiler*.  The issue raised regarding the directories for those logs has 
been fixed so my only comment there is that the diagnostics for failure to 
open those files have several problems:

* Diagnostics should not start with a capital letter.

* Use %< and %> as quotes in diagnostics.

* Use %m in the diagnostic to print the error text corresponding to the 
errno value from the failure to open.

* I doubt input_location is particularly meaningful as a location for 
these diagnostics; warning_at with UNKNOWN_LOCATION may be better.

But there's a much more serious issue with logs generated *by libvtv*.  
These appear to use fixed filenames in a fixed directory /tmp/vtv_logs.  
That's always a mistake (I've encountered such trouble with QEMU using 
such a fixed log name in the past).  Not only that, neither O_EXCL nor 
O_NOFOLLOW is used so you have an obvious security hole.  Using such 
global locations is simply never OK; you need to arrange for such failures 
to go somewhere that will never conflict with other users.  You can't 
create a directory in /tmp (because proper ownership and permissions for 
such a shared directory would be the same as for /tmp itself, rather than 
leaving it owned by the first user to create it, and an unprivileged 
process can't ensure that).  I suggest:

* Prefer the current working directory (as used for core dumps) over /tmp 
(but if /tmp is used as a fallback, it needs to be /tmp not a subdirectory 
thereof).

* Name the generated file with a name that includes the program name, its 
pid, and a random string, to reduce the chance of conflicts.

* In any case, always open with O_NOFOLLOW (if defined) and O_EXCL to 
avoid symlink attacks.

-- 
Joseph S. Myers
joseph@codesourcery.com