[Buildroot] [PATCH 0/2 v3] toolchain/wrapper: add more paranoid checks (branch yem/wrapper)

[Buildroot] [PATCH 0/2 v3] toolchain/wrapper: add more paranoid checks (branch yem/wrapper)

[Yann E. MORIN]

Hello All!

This short series introduces more paranoid checks for unsafe paths, and
reports offending options.


Changes v2 -> v3:
  - also handle -idirafter and -iqupte  (Arnout)
  - simplify the code doing the checks  (Thomas)
  - drop stray variables  (Thomas)
  - comment the tricky code  (Thomas)
  - document what unsafe options are; document the checking function

Changes v1 -> v2:
  - don't use a variadic function, use explicit args  (Arnout)
  - -isystem is not always separated from its path  (Arnout)
  - simpler error reporting  (Arnout)
  - add a list of unsafe options and iterate over it; makes it easier to
    handle even more unsafe options


Regards,
Yann E. MORIN.


The following changes since commit 07552de23b95b48412be89ca5c162d8855872206

  tinydtls: fix issue on u_intXX_t being undefined when building with musl (2016-08-29 17:23:41 +0200)


are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to 110620526667c2676631a91f4d223e093896bda9

  toolchain/wrapper: extend paranoid check to -isystem (2016-08-29 17:42:13 +0200)


----------------------------------------------------------------
Yann E. MORIN (2):
      toolchain/wrapper: display options leading to a paranoid failure
      toolchain/wrapper: extend paranoid check to -isystem

 toolchain/toolchain-wrapper.c | 86 ++++++++++++++++++++++++++++++-------------
 1 file changed, 61 insertions(+), 25 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2 v3] toolchain/wrapper: display options leading to a paranoid failure

[Yann E. MORIN]

Current, we only display the path that causes the paranoid failure. This
is sufficient, as we can fail only for -I and -L options, and it is thus
easy to infer from the path, which option is the culprit.

However, we're soon to add a new test for the -isystem option, and then
when a failure occurs, we would not know whether it was because of -I or
-isystem. Being able to differentiate both can be hugely useful to
track down the root cause for the unsafe path.

Add two new arguments to the check_unsafe_path() function: one with the
current-or-previous argument, one to specify whether it has the path in
it or not. Print that in the error message, instead of just the path.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>

---
---
Changes v2 -> v3:
  - invert test to gain one indentation level  (Thomas)
  - remove stray variables from v1  (Thomas)
  - add doc to the function  (Thomas)
  - add a comment about the "' '" trick  (Thomas)

Changes v1 -> v2;
  - don't use a variadic function; use explicit argumetns  (Arnout)
  - print it on a single line  (Arnout)
---
 toolchain/toolchain-wrapper.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index 887058f..6259dac 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -80,7 +80,21 @@ static char *predef_args[] = {
 #endif
 };
 
-static void check_unsafe_path(const char *path, int paranoid)
+/* Check if path is unsafe for cross-compilation. Unsafe paths are those
+ * pointing to the standard native include or library paths.
+ *
+ * We print the arguments leading to the failure. For some options, gcc
+ * accepts the path to be concatenated to the argument (e.g. -I/foo/bar)
+ * or separated (e.g. -I /foo/bar). In the first case, we need only print
+ * the argument as it already contains the path (arg_has_path), while in
+ * the second case we need to print both (!arg_has_path).
+ *
+ * If paranoid, exit in error instead of just printing a warning.
+ */
+static void check_unsafe_path(const char *arg,
+			      const char *path,
+			      int paranoid,
+			      int arg_has_path)
 {
 	char **c;
 	static char *unsafe_paths[] = {
@@ -88,14 +102,17 @@ static void check_unsafe_path(const char *path, int paranoid)
 	};
 
 	for (c = unsafe_paths; *c != NULL; c++) {
-		if (!strncmp(path, *c, strlen(*c))) {
-			fprintf(stderr, "%s: %s: unsafe header/library path used in cross-compilation: '%s'\n",
-				program_invocation_short_name,
-				paranoid ? "ERROR" : "WARNING", path);
-			if (paranoid)
-				exit(1);
+		if (strncmp(path, *c, strlen(*c)))
 			continue;
-		}
+		fprintf(stderr,
+			"%s: %s: unsafe header/library path used in cross-compilation: '%s%s%s'\n",
+			program_invocation_short_name,
+			paranoid ? "ERROR" : "WARNING",
+			arg,
+			arg_has_path ? "" : "' '", /* close single-quote, space, open single-quote */
+			arg_has_path ? "" : path); /* so that arg and path are properly quoted. */
+		if (paranoid)
+			exit(1);
 	}
 }
 
@@ -237,9 +254,9 @@ int main(int argc, char **argv)
 			i++;
 			if (i == argc)
 				continue;
-			check_unsafe_path(argv[i], paranoid);
+			check_unsafe_path(argv[i-1], argv[i], paranoid, 0);
 		} else {
-			check_unsafe_path(argv[i] + 2, paranoid);
+			check_unsafe_path(argv[i], argv[i] + 2, paranoid, 1);
 		}
 	}
 
-- 
2.7.4

[Buildroot] [PATCH 2/2 v3] toolchain/wrapper: extend paranoid check to -isystem

[Yann E. MORIN]

Some packages, like libbsd, use -isystem flags to provide so-called
overrides to the system include files. In this particular case, this
is used in a .pc file, then used by antoher package; pkgconf does not
mangle this path; and eventually that other package ends up using
/usr/include/bsd to search for headers.

Our current toolchain wrapper is limited to looking for -I and -L, so
the paranoid check does not kick in.

Furthermore, as noticed by Arnout, there might be a bunch of other
so-unsafe options: -isysroot, -imultilib, -iquote, -idirafter, -iprefix,
-iwithprefix, -iwithprefixbefore; even -B and --sysroot are unsafe.

Extend the paranoid check to be able to check any arbitrary number of
potentially unsafe options:

  - add a list of options to check for, each with their length,
  - iterate over this list until we find a matching unsafe option.

Compared to previously, the list of options include -I and -L (which we
already had) extended with -idirafter, -iquote and -isystem, but leaving
all the others noticed by Arnout away, until we have a reason for
handling them.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>

---
Changes v2 -> v3:
  - also cover -idirafter and -iquote  (Arnout)
  - slightly document what an unsafe option is

Changes v1 -> v2:
  - don't suppose that -isystem is separated from its path  (Arnout)
  - use and iterate over a list of options rather than using a
    succession of strncmp() in the code, which makes it easier to
    check more unsafe options
---
 toolchain/toolchain-wrapper.c | 53 +++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index 6259dac..d8b35a5 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -80,6 +80,26 @@ static char *predef_args[] = {
 #endif
 };
 
+struct unsafe_opt_s {
+	const char *arg;
+	size_t     len;
+};
+
+/* Unsafe options are options that specify a potentialy unsafe path,
+ * that will be checked by check_unsafe_path(), below.
+ *
+ * sizeof() on a string literal includes the terminating \0.
+ */
+#define UNSAFE_OPT(o) { #o, sizeof(#o)-1 }
+static const struct unsafe_opt_s unsafe_opts[] = {
+	UNSAFE_OPT(-I),
+	UNSAFE_OPT(-idirafter).
+	UNSAFE_OPT(-iquote),
+	UNSAFE_OPT(-isystem),
+	UNSAFE_OPT(-L),
+	{ NULL, 0 },
+};
+
 /* Check if path is unsafe for cross-compilation. Unsafe paths are those
  * pointing to the standard native include or library paths.
  *
@@ -239,24 +259,23 @@ int main(int argc, char **argv)
 
 	/* Check for unsafe library and header paths */
 	for (i = 1; i < argc; i++) {
-
-		/* Skip options that do not start with -I and -L */
-		if (strncmp(argv[i], "-I", 2) && strncmp(argv[i], "-L", 2))
-			continue;
-
-		/* We handle two cases: first the case where -I/-L and
-		 * the path are separated by one space and therefore
-		 * visible as two separate options, and then the case
-		 * where they are stuck together forming one single
-		 * option.
-		 */
-		if (argv[i][2] == '\0') {
-			i++;
-			if (i == argc)
+		const struct unsafe_opt_s *opt;
+		for (opt=unsafe_opts; opt->arg; opt++ ) {
+			/* Skip any non-unsafe option. */
+			if (strncmp(argv[i], opt->arg, opt->len))
 				continue;
-			check_unsafe_path(argv[i-1], argv[i], paranoid, 0);
-		} else {
-			check_unsafe_path(argv[i], argv[i] + 2, paranoid, 1);
+
+			/* Handle both cases:
+			 *  - path is a separate argument,
+			 *  - path is concatenated with option.
+			 */
+			if (argv[i][opt->len] == '\0') {
+				i++;
+				if (i == argc)
+					break;
+				check_unsafe_path(argv[i-1], argv[i], paranoid, 0);
+			} else
+				check_unsafe_path(argv[i], argv[i] + opt->len, paranoid, 1);
 		}
 	}
 
-- 
2.7.4

[Buildroot] [PATCH 1/2 v3] toolchain/wrapper: display options leading to a paranoid failure

[Thomas Petazzoni]

Hello,

On Mon, 29 Aug 2016 17:53:58 +0200, Yann E. MORIN wrote:
> Current, we only display the path that causes the paranoid failure. This
> is sufficient, as we can fail only for -I and -L options, and it is thus
> easy to infer from the path, which option is the culprit.
> 
> However, we're soon to add a new test for the -isystem option, and then
> when a failure occurs, we would not know whether it was because of -I or
> -isystem. Being able to differentiate both can be hugely useful to
> track down the root cause for the unsafe path.
> 
> Add two new arguments to the check_unsafe_path() function: one with the
> current-or-previous argument, one to specify whether it has the path in
> it or not. Print that in the error message, instead of just the path.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> 
> ---
> ---
> Changes v2 -> v3:
>   - invert test to gain one indentation level  (Thomas)
>   - remove stray variables from v1  (Thomas)
>   - add doc to the function  (Thomas)
>   - add a comment about the "' '" trick  (Thomas)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2 v3] toolchain/wrapper: extend paranoid check to -isystem

[Thomas Petazzoni]

Hello,

On Mon, 29 Aug 2016 17:53:59 +0200, Yann E. MORIN wrote:

> +struct unsafe_opt_s {
> +	const char *arg;
> +	size_t     len;
> +};
> +
> +/* Unsafe options are options that specify a potentialy unsafe path,
> + * that will be checked by check_unsafe_path(), below.
> + *
> + * sizeof() on a string literal includes the terminating \0.
> + */
> +#define UNSAFE_OPT(o) { #o, sizeof(#o)-1 }

I was wondering if this was really necessary. You could also have done
just:

static const char *unsafe_opts[] = {
	"-I", "-L", "-isystem", ...
}

and then use strlen() in the loop. But your solution has the advantage
that the string length is calculated once for all, so OK.

> +static const struct unsafe_opt_s unsafe_opts[] = {
> +	UNSAFE_OPT(-I),
> +	UNSAFE_OPT(-idirafter).

This definitely couldn't built: it should have a comma at the end of
the line, not a dot. I've fixed this up and applied (after testing a
bit that the behavior looks sane).

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com