This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Wed, Aug 7, 2013 at 6:03 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> Looking at the patch as committed, there seems to be some confusion about
> the nature of the --enable-vtable-verify configure option.

Yes, there is a bit.

>
> 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.

The original intent of this flag was that if --enable-vtable-verify
was NOT used, then NOTHING having to do with vtable verification would
be built anywhere in the compiler, i.e. the binaries, libraries, etc.
would not contain anything that wasn't there before the vtable
verification patch was committed.  The idea (and implementation) has
evolved a bit, and I am not sure what the "right" way to handle this
option is at the moment.

>
> 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.

I recently submitted a patch that adds the documentation for my
current understanding of the way this option works.

> 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).

Actually if you ever want to use the feature with your compiler you
should build your compiler with --enable-vtable-verify.  This will, as
you noted,  insert calls in libstdc++ to build the verification data
structures and to verify the virtual calls in libstdc++ itself.
However libstdc++ itself contains 'stub' (do-nothing) versions of the
functions that build the data structures and perform the verification.
 So if you want to turn on verification with libstdc++, you link it
with libvtv (which contains the "real" versions of those functions)
and if you don't want verification with libstdc++ you just don't link
in libvtv.  There is no need to multiple versions of libstdc++.

>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.

I supposed the libgcc files could be built all the time (on systems
that support libvtv).  Would there be any down side to this?

> 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 libstdc++ ABI has been enhanced to export the vtable verification
functions for which it contains stub versions (otherwise they could
never be overwritten by the versions in libvtv).  Other than that the
libstdc++ ABI exports the same symbols at the same symbol versions
with the same semantics.  I believe this export is unconditional.

> 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.

I recently submitted a patch to fix these issues.

>
> 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.
>

For certain use cases the current working directory is not our
preferred place to put the log files.
I have recently submitted a patch that tries to use environment
variables to determine where to put the log files.  It first checks to
see if the user has defined VTV_LOGS_DIR, in which case it will use
that.  If that fails, it tries to find and use HOME.  If that also
fails, it falls back on using the working directory.  I hope that is
ok?

I have modified the call to open to take O_NOFOLLOW, but O_EXCL will
do the wrong thing, as we sometimes wish to append to the log files
and O_EXCL fails if you attempt to open an existing file (according to
the documentation I read).


> --
> Joseph S. Myers
> joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]