Port of VTV for Cygwin and MinGW

Discussion:

Patrick Wollgast

2014-08-28 11:03:40 UTC

This patch contains a port of VTV -fvtable-verify=std for Cygwin and MinGW.

Since weak symbols on Windows and Linux are implemented differently, and
VTV should have the possibility to be switched on and off, the structure
of the feature had to be modified.
On Linux libstdc++ contains the weak stub functions of VTV. For Cygwin
and MinGW they have been removed, due to the difference of weak symbols.
On Linux and on Windows libstdc++ itself gets build with
-fvtable-verify=std. Since libvtv gets build after libstdc++, and
libstdc++ doesn't contain the stub functions any more, 'undefined
reference' errors are thrown during linking of libstdc++. To prevent
these errors during the linking process a libvtv-0.dll gets build from
the stub functions before libstdc++-6.dll is linked.
At the end of the build process two VTV dlls have been build. One is
called libvtv-0.dll, containing the real functions, the other is called
libvtv_stubs-0.dll, containing the stub functions. Depending on whether
libvtv-0.dll is first found in the dll search path or
libvtv_stubs-0.dll, renamed to libvtv-0.dll, the real functions or the
stub functions are used.

Testing:
The test builds were configured the following way:
Linux 64bit (from patched and unpatched trunk):
/path/to/configure --prefix=/prefix/gcc-vtv-bin-64
--enable-libstdcxx-threads --enable-vtable-verify=yes
MinGW 32bit cross compiled:
/path/to/configure --target=i686-w64-mingw32
--prefix=/prefix/mingw-vtv-bin-32 --with-gnu-ld --with-gnu-as
--enable-fully-dynamic-string --disable-multilib
--enable-libstdcxx-threads --enable-vtable-verify=yes
MinGW 64bit cross compiled:
/path/to/configure --target=x86_64-w64-mingw32
--prefix=/prefix/mingw-vtv-bin-64 --with-gnu-ld --with-gnu-as
--enable-fully-dynamic-string --disable-multilib
--enable-libstdcxx-threads --enable-vtable-verify=yes
Cygwin 64bit:
/path/to/configure --enable-languages=c,c++ --enable-libstdcxx-threads
--enable-vtable-verify=yes

At Linux the patched and unpatched version resulted in the same number
of passed tests with 'make check-target-libvtv'.

Since MinGW was cross compiled the test cases couldn't be built and run
with 'make check-target-libvtv'. Therefore they were built with the
attached makefiles and tested afterwards on Windows 7 64bit. Some test
cases contain Linux specific parts and weren't tested. See the makefiles
for further information. Additionally virtual_func_test_min_UAF.cpp was
also built and tested. All built tests passed.

Cygwin was just tested on gcc 4.9.0, because the current trunk isn't
building for me. Even the clean trunk without the patch attached to this
mail. On Cygwin with gcc 4.9.0 VTV worked.

Besides the test cases Botan was also built and tested (gcc 4.9.0) with
MinGW 32bit and VTV.

regards

Patrick Wollgast

2014-09-11 04:12:47 UTC

Permalink

Ping for https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02559.html

Also added Caroline Tice, as libvtv maintainer, to cc and attached
virtual_func_test_min_UAF.cpp, which I forgot in the original mail.

Patrick

Post by Patrick Wollgast
This patch contains a port of VTV -fvtable-verify=std for Cygwin and MinGW.
Since weak symbols on Windows and Linux are implemented differently, and
VTV should have the possibility to be switched on and off, the structure
of the feature had to be modified.
On Linux libstdc++ contains the weak stub functions of VTV. For Cygwin
and MinGW they have been removed, due to the difference of weak symbols.
On Linux and on Windows libstdc++ itself gets build with
-fvtable-verify=std. Since libvtv gets build after libstdc++, and
libstdc++ doesn't contain the stub functions any more, 'undefined
reference' errors are thrown during linking of libstdc++. To prevent
these errors during the linking process a libvtv-0.dll gets build from
the stub functions before libstdc++-6.dll is linked.
At the end of the build process two VTV dlls have been build. One is
called libvtv-0.dll, containing the real functions, the other is called
libvtv_stubs-0.dll, containing the stub functions. Depending on whether
libvtv-0.dll is first found in the dll search path or
libvtv_stubs-0.dll, renamed to libvtv-0.dll, the real functions or the
stub functions are used.
/path/to/configure --prefix=/prefix/gcc-vtv-bin-64
--enable-libstdcxx-threads --enable-vtable-verify=yes
/path/to/configure --target=i686-w64-mingw32
--prefix=/prefix/mingw-vtv-bin-32 --with-gnu-ld --with-gnu-as
--enable-fully-dynamic-string --disable-multilib
--enable-libstdcxx-threads --enable-vtable-verify=yes
/path/to/configure --target=x86_64-w64-mingw32
--prefix=/prefix/mingw-vtv-bin-64 --with-gnu-ld --with-gnu-as
--enable-fully-dynamic-string --disable-multilib
--enable-libstdcxx-threads --enable-vtable-verify=yes
/path/to/configure --enable-languages=c,c++ --enable-libstdcxx-threads
--enable-vtable-verify=yes
At Linux the patched and unpatched version resulted in the same number
of passed tests with 'make check-target-libvtv'.
Since MinGW was cross compiled the test cases couldn't be built and run
with 'make check-target-libvtv'. Therefore they were built with the
attached makefiles and tested afterwards on Windows 7 64bit. Some test
cases contain Linux specific parts and weren't tested. See the makefiles
for further information. Additionally virtual_func_test_min_UAF.cpp was
also built and tested. All built tests passed.
Cygwin was just tested on gcc 4.9.0, because the current trunk isn't
building for me. Even the clean trunk without the patch attached to this
mail. On Cygwin with gcc 4.9.0 VTV worked.
Besides the test cases Botan was also built and tested (gcc 4.9.0) with
MinGW 32bit and VTV.
regards

--
Beste GrÃŒÃe,
Patrick

Caroline Tice

2014-09-12 22:43:41 UTC

Permalink

First attempt to send this failed.

Hi Patrick,
Mostly your patch looks OK to me, though there are a couple of serious issues (mentioned below). Most of my comments are for formatting stuff. Once you have fixed these issues, let me know and I'll look at it again. But someone else will still have to approve the parts of this patch that are outside the libvtv directory (I believe).
-- Caroline Tice
In changes to gcc/config/i386/cygwin.h mingw-w64.h and mingw32.h, you forgot to handle the "fvtable-verify=preinit" options. fvtable-veriy=preinit should cause vtv_start_preinit.o to be added to the STARTFILE_SPEC and vtv_end_preinit.o to be added to the ENDFILE_SPEC (as in gcc/config/gnu-user.h). I expect you will also need to add it to your LIB_SPEC definitions in those config files.
in libgcc/config.host, the indentation looks wrong on the line 660 (where you add the extra parts for vtable verification for the case "i[34567]86-*-mingw*)". It also looks wrong on line 709 (again, adding extra parts, for case "x86_64-*-mingw*)". The rest of the changes in that file look ok, but someone else will need to approve them.
The changes in libgcc/Makefile.in and gcc/varasm.c look ok to me, but someone will will have to approve them since I don't have authority to approve changes there.
Your change in Makefile.am looks ok to me; your changes in vtv_stubs.cc look ok, except that you appear to have messed up the indentations in the function headers (both for the declarations and the actual functions). Also there is a typo in your comment: 'build' should be 'built'. But content-wise, the change looks fine. Again, someone else will have to actually approve these changes since I do not have that authority.
in libstdc++-v3/src/Makefile.am also looks ok to me; someone else will have to give final approval.
vtv_stubs_sources = \
vtv_start.c \
vtv_stubs.cc \
vtv_end.c
the rest of the changes in that file look good.
Why did you make a copy of obstack.c in libvtv rather than using the one in libiberty? It would be better not to make a second copy of the source file if that can be avoided...
lines 207-213: Fix the indentation on the second line of call to VirtualAlloc.
#if defined (__CYGWIN__) || defined (__MINGW32__)
if ((allocated = VirtualAlloc(NULL, size, MEM_RESERVE|MEM_COMMIT,
PAGE_READWRITE)) == 0)
#else
if ((allocated = mmap (NULL, size, PROT_READ | PROT_WRITE,"
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) == 0)
#endif
Remove extra blank line at line 216.
Your version of the function read_section_offset_and_length has several lines that exceed the 80 character limit; please fix that. Your function iterate_modules also has one or two lines that are too long, and it needs a function comment.
#ifdef __MINGW32__
mkdir (logs_prefix);
#else
mkdir (logs_prefix, S_IRWXU);
#endif
was there a reason for this, or is this an accident (in which case the second occurrence should be removed)?

Post by Patrick Wollgast
Ping for https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02559.html
Also added Caroline Tice, as libvtv maintainer, to cc and attached
virtual_func_test_min_UAF.cpp, which I forgot in the original mail.
Patrick

--
Beste Grüße,
Patrick

Patrick Wollgast

2014-09-18 22:23:58 UTC

Permalink

Added Benjamin De Kosnik as a c++ runtime libs maintainer and Kai Tietz
as Windows/Cygwin/MinGW maintainer.

In changes to gcc/config/i386/cygwin.h mingw-w64.h and mingw32.h, you forgot to handle the "fvtable-verify=preinit" options. fvtable-veriy=preinit should cause vtv_start_preinit.o to be added to the STARTFILE_SPEC and vtv_end_preinit.o to be added to the ENDFILE_SPEC (as in gcc/config/gnu-user.h). I expect you will also need to add it to your LIB_SPEC definitions in those config files.

Like discussed via email I set preinit=std. This required additional
changes in gcc/cp/vtable-class-hierarchy.c.

in libgcc/config.host, the indentation looks wrong on the line 660 (where you add the extra parts for vtable verification for the case "i[34567]86-*-mingw*)". It also looks wrong on line 709 (again, adding extra parts, for case "x86_64-*-mingw*)". The rest of the changes in that file look ok, but someone else will need to approve them.

Indentation fixed.

The changes in libgcc/Makefile.in and gcc/varasm.c look ok to me, but someone will will have to approve them since I don't have authority to approve changes there.
Your change in Makefile.am looks ok to me; your changes in vtv_stubs.cc look ok, except that you appear to have messed up the indentations in the function headers (both for the declarations and the actual functions). Also there is a typo in your comment: 'build' should be 'built'. But content-wise, the change looks fine. Again, someone else will have to actually approve these changes since I do not have that authority.

Typo and indentation fixed.

in libstdc++-v3/src/Makefile.am also looks ok to me; someone else will have to give final approval.
vtv_stubs_sources = \
vtv_start.c \
vtv_stubs.cc \
vtv_end.c

Indentation fixed.

the rest of the changes in that file look good.
Why did you make a copy of obstack.c in libvtv rather than using the one in libiberty? It would be better not to make a second copy of the source file if that can be avoided...

I removed obstack.c from libvtv and added the changes from
libvtv/obstack.c to libiberty/obstack.c with #ifdefs.

lines 207-213: Fix the indentation on the second line of call to VirtualAlloc.
#if defined (__CYGWIN__) || defined (__MINGW32__)
if ((allocated = VirtualAlloc(NULL, size, MEM_RESERVE|MEM_COMMIT,
PAGE_READWRITE)) == 0)
#else
if ((allocated = mmap (NULL, size, PROT_READ | PROT_WRITE,"
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) == 0)
#endif
Remove extra blank line at line 216.

Line removed and indentation fixed.

Your version of the function read_section_offset_and_length has several lines that exceed the 80 character limit; please fix that. Your function iterate_modules also has one or two lines that are too long, and it needs a function comment.

Character per line are now correct and comment added.

#ifdef __MINGW32__
mkdir (logs_prefix);
#else
mkdir (logs_prefix, S_IRWXU);
#endif
was there a reason for this, or is this an accident (in which case the second occurrence should be removed)?

Should have been 'log_dir' instead of 'logs_prefix' the 2nd time. Fixed now.

regards
Patrick

Caroline Tice

2014-09-23 06:16:26 UTC

Permalink

Ok, your patch looks OK to me, but I can only approve the libvtv file
changes. The changes in the other files also seem ok to me, but
someone else will have to approve the modifications in them:

gcc/config/i386/cygwin.h
gcc/config/i386/mingw-w64.h
gcc/config/i386/mingw32.h
gcc/cp/vtable-class-hierarchy.c
gcc/varasm.c

libgcc/Makefile.in
libgcc/config.host

libiberty/obstack.c

libstdc++-v3/acinclude.m4
libstdc++-v3/libsupc++/Makefile.am
libstdc++-v3/libsupc++/vtv_stubs.cc

-- Caroline Tice
***@google.com

On Thu, Sep 18, 2014 at 3:23 PM, Patrick Wollgast

Post by Patrick Wollgast
Added Benjamin De Kosnik as a c++ runtime libs maintainer and Kai Tietz
as Windows/Cygwin/MinGW maintainer.

Like discussed via email I set preinit=std. This required additional
changes in gcc/cp/vtable-class-hierarchy.c.

Indentation fixed.

Typo and indentation fixed.

in libstdc++-v3/src/Makefile.am also looks ok to me; someone else will have to give final approval.
vtv_stubs_sources = \
vtv_start.c \
vtv_stubs.cc \
vtv_end.c

Indentation fixed.

I removed obstack.c from libvtv and added the changes from
libvtv/obstack.c to libiberty/obstack.c with #ifdefs.

Line removed and indentation fixed.

Character per line are now correct and comment added.

#ifdef __MINGW32__
mkdir (logs_prefix);
#else
mkdir (logs_prefix, S_IRWXU);
#endif
was there a reason for this, or is this an accident (in which case the second occurrence should be removed)?

Should have been 'log_dir' instead of 'logs_prefix' the 2nd time. Fixed now.
regards
Patrick

Jonathan Wakely

2014-09-23 10:22:35 UTC

Permalink

Index: libstdc++-v3/acinclude.m4
===================================================================
--- libstdc++-v3/acinclude.m4 (Revision 214408)
+++ libstdc++-v3/acinclude.m4 (Arbeitskopie)
@@ -2321,7 +2321,17 @@ AC_DEFUN([GLIBCXX_ENABLE_VTABLE_VERIFY],
AC_MSG_RESULT([$enable_vtable_verify])
if test $enable_vtable_verify = yes; then
+ case ${target_os} in
+ cygwin*|mingw32*)
+ VTV_CXXFLAGS="-fvtable-verify=std -Wl,-lvtv,-u_vtable_map_vars_start,-u_vtable_map_vars_end"
+ vtv_cygmin="yes"
+ ;;
+ *)
VTV_CXXFLAGS="-fvtable-verify=std -Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end"
+ vtv_cygmin="no"
+ ;;
+ esac
+ AM_CONDITIONAL(VTV_CYGMIN, test $vtv_cygmin = yes)
VTV_PCH_CXXFLAGS="-fvtable-verify=std"
VTV_CXXLINKFLAGS="-L${toplevel_builddir}/libvtv/.libs -Wl,--rpath -Wl,${toplevel_builddir}/libvtv/.libs"
else

The lines between case and esac should be indented by four more
spaces.

Apart from that the libstdc++ changes are OK and I'll approve them.

Patrick Wollgast

2014-09-24 22:25:15 UTC

Permalink

Post by Jonathan Wakely

Index: libstdc++-v3/acinclude.m4
===================================================================
--- libstdc++-v3/acinclude.m4 (Revision 214408)
+++ libstdc++-v3/acinclude.m4 (Arbeitskopie)
@@ -2321,7 +2321,17 @@ AC_DEFUN([GLIBCXX_ENABLE_VTABLE_VERIFY],
AC_MSG_RESULT([$enable_vtable_verify])
if test $enable_vtable_verify = yes; then
+ case ${target_os} in
+ cygwin*|mingw32*)
+ VTV_CXXFLAGS="-fvtable-verify=std
-Wl,-lvtv,-u_vtable_map_vars_start,-u_vtable_map_vars_end"
+ vtv_cygmin="yes"
+ ;;
+ *)
VTV_CXXFLAGS="-fvtable-verify=std
-Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end"
+ vtv_cygmin="no"
+ ;;
+ esac
+ AM_CONDITIONAL(VTV_CYGMIN, test $vtv_cygmin = yes)
VTV_PCH_CXXFLAGS="-fvtable-verify=std"
VTV_CXXLINKFLAGS="-L${toplevel_builddir}/libvtv/.libs -Wl,--rpath
-Wl,${toplevel_builddir}/libvtv/.libs"
else

The lines between case and esac should be indented by four more
spaces.
Apart from that the libstdc++ changes are OK and I'll approve them.

I've changed the indentation at the newly attached patch.

Post by Jonathan Wakely
Ok, your patch looks OK to me, but I can only approve the libvtv file
changes. The changes in the other files also seem ok to me, but
gcc/config/i386/cygwin.h
gcc/config/i386/mingw-w64.h
gcc/config/i386/mingw32.h
gcc/cp/vtable-class-hierarchy.c
gcc/varasm.c
libgcc/Makefile.in
libgcc/config.host
libiberty/obstack.c
libstdc++-v3/acinclude.m4
libstdc++-v3/libsupc++/Makefile.am
libstdc++-v3/libsupc++/vtv_stubs.cc

I've added Ian Lance Taylor to cc of this mail.

Thank you both for the reviews!

regards
Patrick

Kai Tietz

2014-09-27 10:50:33 UTC

Permalink

Hi Patrick,

the mingw/cygwin part your patch looks fine to me. Nevertheless I
have one question regarding to you. Do you have FSF papers for gcc
already? As I asked an overseer and he didn't found you on the list.

Regards,
Kai

Patrick Wollgast

2014-10-09 13:52:22 UTC

Permalink

Hi Patrick,
the mingw/cygwin part your patch looks fine to me. Nevertheless I
have one question regarding to you. Do you have FSF papers for gcc
already? As I asked an overseer and he didn't found you on the list.
Regards,
Kai

The papers FSF have been taken care of, and the signed papers have been
exchanged.

A short recap:

Mail with the latest patch and changelog:
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02200.html

Approved:
* gcc/config/i386/*
* libstdc++-v3/*
* libvtv/*

Not approved:
* gcc/cp/vtable-class-hierarchy.c
* gcc/varasm.c
* libgcc/Makefile.in
* libgcc/config.host
* libiberty/obstack.c

Regards,
Patrick

Kai Tietz

2014-10-09 14:42:42 UTC

Permalink

Post by Patrick Wollgast

Hi Patrick,
the mingw/cygwin part your patch looks fine to me. Nevertheless I
have one question regarding to you. Do you have FSF papers for gcc
already? As I asked an overseer and he didn't found you on the list.
Regards,
Kai

The papers FSF have been taken care of, and the signed papers have been
exchanged.
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02200.html
* gcc/config/i386/*
* libstdc++-v3/*
* libvtv/*
* gcc/cp/vtable-class-hierarchy.c

Index: gcc/cp/vtable-class-hierarchy.c
===================================================================
--- gcc/cp/vtable-class-hierarchy.c (Revision 214408)
+++ gcc/cp/vtable-class-hierarchy.c (Arbeitskopie)
@@ -1182,7 +1182,7 @@ vtv_generate_init_routine (void)
TREE_STATIC (vtv_fndecl) = 1;
TREE_USED (vtv_fndecl) = 1;
DECL_PRESERVE_P (vtv_fndecl) = 1;
- if (flag_vtable_verify == VTV_PREINIT_PRIORITY)
+ if (flag_vtable_verify == VTV_PREINIT_PRIORITY && !TARGET_PECOFF)

You need to check that TARGET_PECOFF is defined. Otherwise you break
compilation for none i386 targets.

DECL_STATIC_CONSTRUCTOR (vtv_fndecl) = 0;

gimplify_function_tree (vtv_fndecl);
@@ -1190,7 +1190,7 @@ vtv_generate_init_routine (void)

cgraph_process_new_functions ();

- if (flag_vtable_verify == VTV_PREINIT_PRIORITY)
+ if (flag_vtable_verify == VTV_PREINIT_PRIORITY && !TARGET_PECOFF)

See above. Likewise
assemble_vtv_preinit_initializer (vtv_fndecl);

}

Post by Patrick Wollgast
* gcc/varasm.c

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c (Revision 214408)
+++ gcc/varasm.c (Arbeitskopie)
@@ -2165,6 +2165,33 @@ assemble_variable (tree decl, int top_le
DECL_NAME (decl));
in_section = sect;
#else
+ /* Neither OBJECT_FORMAT_PE, nor OBJECT_FORMAT_COFF is set here.
+ Therefore the following check is used.
+ In case a the target is PE or COFF a comdat group section
+ is created, e.g. .vtable_map_vars$foo. The linker places
+ everything in .vtable_map_vars at the end.
+
+ A fix could be made in
+ gcc/config/i386/winnt.c: i386_pe_unique_section. */
+ if (TARGET_PECOFF)

You need to test, if TARGET_PECOFF is defined!

+ {
+ char *name;
+
+ if (TREE_CODE (DECL_NAME (decl)) == IDENTIFIER_NODE)
+ name = ACONCAT ((sect->named.name, "$",
+ IDENTIFIER_POINTER (DECL_NAME (decl)), NULL));
+ else
+ name = ACONCAT ((sect->named.name, "$",
+ IDENTIFIER_POINTER (DECL_COMDAT_GROUP (DECL_NAME (decl))),
+ NULL));
+
+ targetm.asm_out.named_section (name,
+ sect->named.common.flags
+ | SECTION_LINKONCE,

Here it seems to me that you have some whitespace issues,

+ DECL_NAME (decl));
+ in_section = sect;
+ }
+ else
switch_to_section (sect);
#endif

Post by Patrick Wollgast
* libgcc/Makefile.in

Looks ok to me.

Post by Patrick Wollgast
* libgcc/config.host

Looks fine to me, too.

Post by Patrick Wollgast
* libiberty/obstack.c

Why you use instead of C-runtime exit/abort-functions the
platform-functions to terminate the process. This looks to me like
useless change. For cygwin this might be even wrong in some aspects.
What is the reasoning for this change?

Another note I have about re-implementation of mprotect in ---
libvtv/vtv_malloc.cc. Why you need that? it is already part of
libgcc for mingw. And for cygwin this function is part of cygwin's
library itself. So why re-implementing it here?

Post by Patrick Wollgast
Regards,
Patrick

Regards,
Kai