diff mbox

[06/12] gcc/4.9: add patch to warn about unsafe header paths

Message ID 1408540005-26934-7-git-send-email-thomas.petazzoni@free-electrons.com
State Superseded
Headers show

Commit Message

Thomas Petazzoni Aug. 20, 2014, 1:06 p.m. UTC 
This commit adds a patch to gcc borrowed from CodeSourcery/Yocto that
warns about unsafe include paths (i.e /usr/include,
/usr/local/include, etc.). The patch was adapted to gcc 4.9.1, and
modified to support the BR_COMPILER_PARANOID_UNSAFE_PATH environment
variable to error out instead of just warn when unsafe paths are
used. Even though erroring out can be chosen by passing
-Werror=poison-system-directories, we are not sure this option in
CFLAGS will always be passed, so having an environment variable
guarantees it will always be passed, and also allows to have an
identical behavior to the external toolchain wrapper.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../4.9.1/910-gcc-poison-system-directories.patch  | 207 +++++++++++++++++++++
 1 file changed, 207 insertions(+)
 create mode 100644 package/gcc/4.9.1/910-gcc-poison-system-directories.patch

Comments

Yann E. MORIN Sept. 10, 2014, 8:08 p.m. UTC | #1 
Thomas, All,

On 2014-08-20 15:06 +0200, Thomas Petazzoni spake thusly:
> +Index: b/gcc/gcc.c
> +===================================================================
> +--- a/gcc/gcc.c
> ++++ b/gcc/gcc.c
[--SNIP--]
> +@@ -4028,6 +4030,12 @@
> +       gcc_assert (!compare_debug_opt);
> +     }
> + 
> ++  temp = getenv("BR_COMPILER_PARANOID_UNSAFE_PATH");
> ++  if (temp && strlen(temp) > 0)
> ++    {
> ++      save_switch("-Werror=poison-system-directories", 0, NULL, false, true);

Here you set the --error-poison-system-direcotries for the linker...

> ++    }
> ++
> +   /* Set up the search paths.  We add directories that we expect to
> +      contain GNU Toolchain components before directories specified by
> +      the machine description so that we will find GNU components (like
> +Index: b/gcc/incpath.c
> +===================================================================
> +--- a/gcc/incpath.c
> ++++ b/gcc/incpath.c
> +@@ -28,6 +28,7 @@
> + #include "intl.h"
> + #include "incpath.h"
> + #include "cppdefault.h"
> ++#include "diagnostic-core.h"
> + 
> + /* Microsoft Windows does not natively support inodes.
> +    VMS has non-numeric inodes.  */
> +@@ -382,6 +383,24 @@
> + 	}
> +       fprintf (stderr, _("End of search list.\n"));
> +     }
> ++
> ++#ifdef ENABLE_POISON_SYSTEM_DIRECTORIES
> ++  if (flag_poison_system_directories)
> ++    {
> ++       struct cpp_dir *p;
> ++
> ++       for (p = heads[QUOTE]; p; p = p->next)
> ++         {
> ++          if ((!strncmp (p->name, "/usr/include", 12))
> ++              || (!strncmp (p->name, "/usr/local/include", 18))
> ++              || (!strncmp (p->name, "/usr/X11R6/include", 18)))
> ++            warning (OPT_Wpoison_system_directories,
> ++                     "include location \"%s\" is unsafe for "
> ++                     "cross-compilation",
> ++                     p->name);

But you do not handle include paths. We should error out if
BR_COMPILER_PARANOID_UNSAFE_PATH is set and non-empty, no?

BTW, I commented only one vrsion of each component, and the comments
apply to the other versions, of course.

Regards,
Yann E. MORIN.

> ++         }
> ++    }
> ++#endif
> + }
> + 
> + /* Use given -I paths for #include "..." but not #include <...>, and
> -- 
> 2.0.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Thomas Petazzoni Sept. 10, 2014, 8:24 p.m. UTC | #2 
Dear Yann E. MORIN,

On Wed, 10 Sep 2014 22:08:25 +0200, Yann E. MORIN wrote:

> > ++  temp = getenv("BR_COMPILER_PARANOID_UNSAFE_PATH");
> > ++  if (temp && strlen(temp) > 0)
> > ++    {
> > ++      save_switch("-Werror=poison-system-directories", 0, NULL, false, true);
> 
> Here you set the --error-poison-system-direcotries for the linker...

No, that's not what's happening here. What's happening here is that if
BR_COMPILER_PARANOID_UNSAFE_PATH is set and non-empty, then we add
-Werror=poison-system-directories to the gcc options. This makes gcc
behave as if -Werror=poison-system-directories was passed on the
command line.

The idea is that it's hard for Buildroot to guarantee that
-Werror=poison-system-directories in the CFLAGS will *always* be passed
by all packages to the build of all files. So having an environment
variable that forces gcc to add this option by itself makes sure all
files will be compiled with this behavior.


> > ++#ifdef ENABLE_POISON_SYSTEM_DIRECTORIES
> > ++  if (flag_poison_system_directories)
> > ++    {
> > ++       struct cpp_dir *p;
> > ++
> > ++       for (p = heads[QUOTE]; p; p = p->next)
> > ++         {
> > ++          if ((!strncmp (p->name, "/usr/include", 12))
> > ++              || (!strncmp (p->name, "/usr/local/include", 18))
> > ++              || (!strncmp (p->name, "/usr/X11R6/include", 18)))
> > ++            warning (OPT_Wpoison_system_directories,
> > ++                     "include location \"%s\" is unsafe for "
> > ++                     "cross-compilation",
> > ++                     p->name);
> 
> But you do not handle include paths.

What? We're precisely handling include paths here.

> We should error out if BR_COMPILER_PARANOID_UNSAFE_PATH is set and
> non-empty, no?

See above, that's exactly what is happening. By default
(BR_COMPILER_PARANOID_UNSAFE_PATH is unset or empty), only warnings are
generated. If BR_COMPILER_PARANOID_UNSAFE_PATH is set and non-empty,
then it will be an error.

You should probably test this, and you'll notice that the warning()
function above actually *errors* out if the OPT_W<foo> warning passed
as first argument has -Werror=<foo> in the command line.

Best regards,

Thomas
Yann E. MORIN Sept. 10, 2014, 10:10 p.m. UTC | #3 
Thomas, All,

On 2014-09-10 22:24 +0200, Thomas Petazzoni spake thusly:
> Dear Yann E. MORIN,
> 
> On Wed, 10 Sep 2014 22:08:25 +0200, Yann E. MORIN wrote:
> 
> > > ++  temp = getenv("BR_COMPILER_PARANOID_UNSAFE_PATH");
> > > ++  if (temp && strlen(temp) > 0)
> > > ++    {
> > > ++      save_switch("-Werror=poison-system-directories", 0, NULL, false, true);
> > 
> > Here you set the --error-poison-system-direcotries for the linker...
> 
> No, that's not what's happening here. What's happening here is that if
> BR_COMPILER_PARANOID_UNSAFE_PATH is set and non-empty, then we add
> -Werror=poison-system-directories to the gcc options. This makes gcc
> behave as if -Werror=poison-system-directories was passed on the
> command line.

Ah, I see now. OK. We tell gcc to treat that warning as an error.
Of course. :-)

Forget my comment.

Regards,
Yann E. MORIN.
diff mbox

Patch

diff --git a/package/gcc/4.9.1/910-gcc-poison-system-directories.patch b/package/gcc/4.9.1/910-gcc-poison-system-directories.patch
new file mode 100644
index 0000000..377ddf5
--- /dev/null
+++ b/package/gcc/4.9.1/910-gcc-poison-system-directories.patch
@@ -0,0 +1,207 @@ 
+From 160397ef3c3331099af028f1b8d3e085b07d88ad Mon Sep 17 00:00:00 2001
+From: Khem Raj <raj.khem@gmail.com>
+Date: Fri, 29 Mar 2013 08:59:00 +0400
+Subject: [PATCH 16/35] gcc: poison-system-directories
+
+Adapted to Buildroot and gcc 4.9.1 by Thomas Petazzoni, especially the
+addition of the BR_COMPILER_PARANOID_UNSAFE_PATH environment variable.
+
+Signed-off-by: Khem Raj <raj.khem@gmail.com>
+Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+
+Upstream-Status: Inappropriate [distribution: codesourcery]
+---
+ gcc/Makefile.in     |    2 +-
+ gcc/common.opt      |    4 ++++
+ gcc/config.in       |    6 ++++++
+ gcc/configure       |   20 ++++++++++++++++++--
+ gcc/configure.ac    |   10 ++++++++++
+ gcc/doc/invoke.texi |    9 +++++++++
+ gcc/gcc.c           |    2 ++
+ gcc/incpath.c       |   19 +++++++++++++++++++
+ 8 files changed, 69 insertions(+), 3 deletions(-)
+
+Index: b/gcc/common.opt
+===================================================================
+--- a/gcc/common.opt
++++ b/gcc/common.opt
+@@ -603,6 +603,10 @@
+ Common Var(pedantic) Warning
+ Issue warnings needed for strict compliance to the standard
+ 
++Wpoison-system-directories
++Common Var(flag_poison_system_directories) Init(1) Warning
++Warn for -I and -L options using system directories if cross compiling
++
+ Wshadow
+ Common Var(warn_shadow) Warning
+ Warn when one local variable shadows another
+Index: b/gcc/config.in
+===================================================================
+--- a/gcc/config.in
++++ b/gcc/config.in
+@@ -138,6 +138,12 @@
+ #endif
+ 
+ 
++/* Define to warn for use of native system header directories */
++#ifndef USED_FOR_TARGET
++#undef ENABLE_POISON_SYSTEM_DIRECTORIES
++#endif
++
++
+ /* Define if you want all operations on RTL (the basic data structure of the
+    optimizer and back end) to be checked for dynamic type safety at runtime.
+    This is quite expensive. */
+Index: b/gcc/configure
+===================================================================
+--- a/gcc/configure
++++ b/gcc/configure
+@@ -928,6 +928,7 @@
+ enable_maintainer_mode
+ enable_link_mutex
+ enable_version_specific_runtime_libs
++enable_poison_system_directories
+ enable_plugin
+ enable_host_shared
+ enable_libquadmath_support
+@@ -1648,6 +1649,8 @@
+   --enable-version-specific-runtime-libs
+                           specify that runtime libraries should be installed
+                           in a compiler-specific directory
++  --enable-poison-system-directories
++                          warn for use of native system header directories
+   --enable-plugin         enable plugin support
+   --enable-host-shared    build host code as shared libraries
+   --disable-libquadmath-support
+@@ -27737,6 +27740,19 @@
+ fi
+ 
+ 
++# Check whether --enable-poison-system-directories was given.
++if test "${enable_poison_system_directories+set}" = set; then :
++  enableval=$enable_poison_system_directories;
++else
++  enable_poison_system_directories=no
++fi
++
++if test "x${enable_poison_system_directories}" = "xyes"; then
++
++$as_echo "#define ENABLE_POISON_SYSTEM_DIRECTORIES 1" >>confdefs.h
++
++fi
++
+ # Substitute configuration variables
+ 
+ 
+Index: b/gcc/configure.ac
+===================================================================
+--- a/gcc/configure.ac
++++ b/gcc/configure.ac
+@@ -5388,6 +5388,16 @@
+                 [specify that runtime libraries should be
+                  installed in a compiler-specific directory])])
+ 
++AC_ARG_ENABLE([poison-system-directories],
++             AS_HELP_STRING([--enable-poison-system-directories],
++                            [warn for use of native system header directories]),,
++             [enable_poison_system_directories=no])
++if test "x${enable_poison_system_directories}" = "xyes"; then
++  AC_DEFINE([ENABLE_POISON_SYSTEM_DIRECTORIES],
++           [1],
++           [Define to warn for use of native system header directories])
++fi
++
+ # Substitute configuration variables
+ AC_SUBST(subdirs)
+ AC_SUBST(srcdir)
+Index: b/gcc/doc/invoke.texi
+===================================================================
+--- a/gcc/doc/invoke.texi
++++ b/gcc/doc/invoke.texi
+@@ -260,6 +260,7 @@
+ -Woverlength-strings  -Wpacked  -Wpacked-bitfield-compat  -Wpadded @gol
+ -Wparentheses  -Wpedantic-ms-format -Wno-pedantic-ms-format @gol
+ -Wpointer-arith  -Wno-pointer-to-int-cast @gol
++-Wno-poison-system-directories @gol
+ -Wredundant-decls  -Wno-return-local-addr @gol
+ -Wreturn-type  -Wsequence-point  -Wshadow @gol
+ -Wsign-compare  -Wsign-conversion -Wfloat-conversion @gol
+@@ -4208,6 +4209,14 @@
+  for most targets, it is made up of code and thus requires the stack
+  to be made executable in order for the program to work properly.
+ 
++@item -Wno-poison-system-directories
++@opindex Wno-poison-system-directories
++Do not warn for @option{-I} or @option{-L} options using system
++directories such as @file{/usr/include} when cross compiling.  This
++option is intended for use in chroot environments when such
++directories contain the correct headers and libraries for the target
++system rather than the host.
++
+ @item -Wfloat-equal
+ @opindex Wfloat-equal
+ @opindex Wno-float-equal
+Index: b/gcc/gcc.c
+===================================================================
+--- a/gcc/gcc.c
++++ b/gcc/gcc.c
+@@ -764,6 +764,8 @@
+    "%{fuse-ld=*:-fuse-ld=%*}\
+     %X %{o*} %{e*} %{N} %{n} %{r}\
+     %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}} " VTABLE_VERIFICATION_SPEC " \
++    %{Wno-poison-system-directories:--no-poison-system-directories}\
++    %{Werror=poison-system-directories:--error-poison-system-directories}\
+     %{static:} %{L*} %(mfwrap) %(link_libgcc) " SANITIZER_EARLY_SPEC " %o\
+     %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
+     %{fcilkplus:%:include(libcilkrts.spec)%(link_cilkrts)}\
+@@ -4028,6 +4030,12 @@
+       gcc_assert (!compare_debug_opt);
+     }
+ 
++  temp = getenv("BR_COMPILER_PARANOID_UNSAFE_PATH");
++  if (temp && strlen(temp) > 0)
++    {
++      save_switch("-Werror=poison-system-directories", 0, NULL, false, true);
++    }
++
+   /* Set up the search paths.  We add directories that we expect to
+      contain GNU Toolchain components before directories specified by
+      the machine description so that we will find GNU components (like
+Index: b/gcc/incpath.c
+===================================================================
+--- a/gcc/incpath.c
++++ b/gcc/incpath.c
+@@ -28,6 +28,7 @@
+ #include "intl.h"
+ #include "incpath.h"
+ #include "cppdefault.h"
++#include "diagnostic-core.h"
+ 
+ /* Microsoft Windows does not natively support inodes.
+    VMS has non-numeric inodes.  */
+@@ -382,6 +383,24 @@
+ 	}
+       fprintf (stderr, _("End of search list.\n"));
+     }
++
++#ifdef ENABLE_POISON_SYSTEM_DIRECTORIES
++  if (flag_poison_system_directories)
++    {
++       struct cpp_dir *p;
++
++       for (p = heads[QUOTE]; p; p = p->next)
++         {
++          if ((!strncmp (p->name, "/usr/include", 12))
++              || (!strncmp (p->name, "/usr/local/include", 18))
++              || (!strncmp (p->name, "/usr/X11R6/include", 18)))
++            warning (OPT_Wpoison_system_directories,
++                     "include location \"%s\" is unsafe for "
++                     "cross-compilation",
++                     p->name);
++         }
++    }
++#endif
+ }
+ 
+ /* Use given -I paths for #include "..." but not #include <...>, and