All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure
@ 2016-08-17 14:42 Yann E. MORIN
  2016-08-17 14:42 ` [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem Yann E. MORIN
  2016-08-24  1:09 ` [Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure Arnout Vandecappelle
  0 siblings, 2 replies; 7+ messages in thread
From: Yann E. MORIN @ 2016-08-17 14:42 UTC (permalink / raw)
  To: buildroot

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.

Make the check_unsafe_path() function accept a variable number of
arguments as a NULL-terminated list, to contain the offending options.

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>
---
 toolchain/toolchain-wrapper.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index 887058f..b8b3cbe 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -22,6 +22,7 @@
 #include <unistd.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <stdarg.h>
 
 #ifdef BR_CCACHE
 static char ccache_path[PATH_MAX];
@@ -80,8 +81,10 @@ static char *predef_args[] = {
 #endif
 };
 
-static void check_unsafe_path(const char *path, int paranoid)
+static void check_unsafe_path(const char *path, int paranoid, ...)
 {
+	va_list ap;
+	int once;
 	char **c;
 	static char *unsafe_paths[] = {
 		"/lib", "/usr/include", "/usr/lib", "/usr/local/include", "/usr/local/lib", NULL,
@@ -92,6 +95,21 @@ static void check_unsafe_path(const char *path, int paranoid)
 			fprintf(stderr, "%s: %s: unsafe header/library path used in cross-compilation: '%s'\n",
 				program_invocation_short_name,
 				paranoid ? "ERROR" : "WARNING", path);
+			va_start(ap, paranoid);
+			once=1;
+			while(1) {
+				char *s = va_arg(ap, char*);
+				if(!s)
+					break;
+				if(once)
+					fprintf(stderr, "%s: options causing the issue:",
+						program_invocation_short_name);
+				once = 0;
+				fprintf(stderr, " '%s'", s);
+			}
+			if(!once)
+				fprintf(stderr, "\n");
+			va_end(ap);
 			if (paranoid)
 				exit(1);
 			continue;
@@ -237,9 +255,9 @@ int main(int argc, char **argv)
 			i++;
 			if (i == argc)
 				continue;
-			check_unsafe_path(argv[i], paranoid);
+			check_unsafe_path(argv[i], paranoid, argv[i-1], argv[i], NULL);
 		} else {
-			check_unsafe_path(argv[i] + 2, paranoid);
+			check_unsafe_path(argv[i] + 2, paranoid, argv[i], NULL);
 		}
 	}
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem
  2016-08-17 14:42 [Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure Yann E. MORIN
@ 2016-08-17 14:42 ` Yann E. MORIN
  2016-08-24  1:18   ` Arnout Vandecappelle
  2016-08-24  1:09 ` [Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure Arnout Vandecappelle
  1 sibling, 1 reply; 7+ messages in thread
From: Yann E. MORIN @ 2016-08-17 14:42 UTC (permalink / raw)
  To: buildroot

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 fo -I and -L, so
the paranoid check does not kick in.

Extend the paranoid check to also look for the -isystem option.

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>
---
 toolchain/toolchain-wrapper.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index b8b3cbe..8a9c3b3 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -241,17 +241,19 @@ 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))
+		/* Skip options that do not start with -I, -isystem or -L */
+		if (   strncmp(argv[i], "-I", 2)
+		    && strncmp(argv[i], "-L", 2)
+		    && strcmp(argv[i], "-isystem"))
 			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.
+		/* We handle two cases: first the case where -I/-L/-isystem
+		 * 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.
+		 * -isystem is necessarily in the first case.
 		 */
-		if (argv[i][2] == '\0') {
+		if (argv[i][2] == '\0' || argv[i][1] == 'i') {
 			i++;
 			if (i == argc)
 				continue;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure
  2016-08-17 14:42 [Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure Yann E. MORIN
  2016-08-17 14:42 ` [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem Yann E. MORIN
@ 2016-08-24  1:09 ` Arnout Vandecappelle
  1 sibling, 0 replies; 7+ messages in thread
From: Arnout Vandecappelle @ 2016-08-24  1:09 UTC (permalink / raw)
  To: buildroot

On 17-08-16 16:42, 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.
> 
> Make the check_unsafe_path() function accept a variable number of
> arguments as a NULL-terminated list, to contain the offending options.
> 
> 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>
> ---
>  toolchain/toolchain-wrapper.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
> index 887058f..b8b3cbe 100644
> --- a/toolchain/toolchain-wrapper.c
> +++ b/toolchain/toolchain-wrapper.c
> @@ -22,6 +22,7 @@
>  #include <unistd.h>
>  #include <stdlib.h>
>  #include <errno.h>
> +#include <stdarg.h>
>  
>  #ifdef BR_CCACHE
>  static char ccache_path[PATH_MAX];
> @@ -80,8 +81,10 @@ static char *predef_args[] = {
>  #endif
>  };
>  
> -static void check_unsafe_path(const char *path, int paranoid)
> +static void check_unsafe_path(const char *path, int paranoid, ...)

 I'm not very happy with using varargs for such a simple case. How about:

static void check_unsafe_path(const char *arg, const char *path,
                              int paranoid, bool arg_includes_path)
...

>  {
> +	va_list ap;
> +	int once;
>  	char **c;
>  	static char *unsafe_paths[] = {
>  		"/lib", "/usr/include", "/usr/lib", "/usr/local/include", "/usr/local/lib", NULL,
> @@ -92,6 +95,21 @@ static void check_unsafe_path(const char *path, int paranoid)
>  			fprintf(stderr, "%s: %s: unsafe header/library path used in cross-compilation: '%s'\n",
>  				program_invocation_short_name,
>  				paranoid ? "ERROR" : "WARNING", path);

and here:

 			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_includes_path ? "" : " ",
				arg_includes_path ? "" : path);

> +			va_start(ap, paranoid);
> +			once=1;
> +			while(1) {
> +				char *s = va_arg(ap, char*);
> +				if(!s)
> +					break;
> +				if(once)
> +					fprintf(stderr, "%s: options causing the issue:",
> +						program_invocation_short_name);
> +				once = 0;
> +				fprintf(stderr, " '%s'", s);
> +			}
> +			if(!once)
> +				fprintf(stderr, "\n");
> +			va_end(ap);
>  			if (paranoid)
>  				exit(1);
>  			continue;
> @@ -237,9 +255,9 @@ int main(int argc, char **argv)
>  			i++;
>  			if (i == argc)
>  				continue;
> -			check_unsafe_path(argv[i], paranoid);
> +			check_unsafe_path(argv[i], paranoid, argv[i-1], argv[i], NULL);

			check_unsafe_path(argv[i-1], argv[i], paranoid, false);

>  		} else {
> -			check_unsafe_path(argv[i] + 2, paranoid);
> +			check_unsafe_path(argv[i] + 2, paranoid, argv[i], NULL);

			check_unsafe_path(argv[i], argv[i] + 2, paranoid, true);


 Regards,
 Arnout


>  		}
>  	}
>  
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem
  2016-08-17 14:42 ` [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem Yann E. MORIN
@ 2016-08-24  1:18   ` Arnout Vandecappelle
  2016-08-24 14:12     ` Yann E. MORIN
  0 siblings, 1 reply; 7+ messages in thread
From: Arnout Vandecappelle @ 2016-08-24  1:18 UTC (permalink / raw)
  To: buildroot

On 17-08-16 16:42, Yann E. MORIN wrote:
> 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 fo -I and -L, so
> the paranoid check does not kick in.
> 
> Extend the paranoid check to also look for the -isystem option.

 While we're at it: -idirafter, -iprefix, -iwithprefix, -iwithprefixbefore,
-isysroot, -imultilib, -iquote.

 And then there is -B, but if someone passes that, it's really broken :-) And
--sysroot, also interesting if that is passed. But I guess these things are
going a bit too far.


> 
> 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>
> ---
>  toolchain/toolchain-wrapper.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
> index b8b3cbe..8a9c3b3 100644
> --- a/toolchain/toolchain-wrapper.c
> +++ b/toolchain/toolchain-wrapper.c
> @@ -241,17 +241,19 @@ 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))
> +		/* Skip options that do not start with -I, -isystem or -L */
> +		if (   strncmp(argv[i], "-I", 2)
> +		    && strncmp(argv[i], "-L", 2)
> +		    && strcmp(argv[i], "-isystem"))
>  			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.
> +		/* We handle two cases: first the case where -I/-L/-isystem
> +		 * 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.
> +		 * -isystem is necessarily in the first case.

 Unfortunately, that's not true. You can pass something like -isystemfoo and it
will add ./foo to the search path. Ain't gcc fun... I'll leave it as an exercise
to the reader to handle that case :-P

 Regards,
 Arnout

>  		 */
> -		if (argv[i][2] == '\0') {
> +		if (argv[i][2] == '\0' || argv[i][1] == 'i') {
>  			i++;
>  			if (i == argc)
>  				continue;
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem
  2016-08-24  1:18   ` Arnout Vandecappelle
@ 2016-08-24 14:12     ` Yann E. MORIN
  2016-08-24 15:23       ` Arnout Vandecappelle
  0 siblings, 1 reply; 7+ messages in thread
From: Yann E. MORIN @ 2016-08-24 14:12 UTC (permalink / raw)
  To: buildroot

Arnout, All,

On 2016-08-24 03:18 +0200, Arnout Vandecappelle spake thusly:
> On 17-08-16 16:42, Yann E. MORIN wrote:
> > 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 fo -I and -L, so
> > the paranoid check does not kick in.
> > 
> > Extend the paranoid check to also look for the -isystem option.
> 
>  While we're at it: -idirafter, -iprefix, -iwithprefix, -iwithprefixbefore,
> -isysroot, -imultilib, -iquote.

Did you meant we should handle all of them now? Are were you listing
them for the future, when we encoutner issues with any if them?

>  And then there is -B, but if someone passes that, it's really broken :-) And
> --sysroot, also interesting if that is passed. But I guess these things are
> going a bit too far.

--sysroot is even more fun, as it can be written: --sysroot=dir , so
we'd need to take care of this as well...

Lotta fun in sight! ;-)

> > 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>
> > ---
> >  toolchain/toolchain-wrapper.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
> > index b8b3cbe..8a9c3b3 100644
> > --- a/toolchain/toolchain-wrapper.c
> > +++ b/toolchain/toolchain-wrapper.c
> > @@ -241,17 +241,19 @@ 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))
> > +		/* Skip options that do not start with -I, -isystem or -L */
> > +		if (   strncmp(argv[i], "-I", 2)
> > +		    && strncmp(argv[i], "-L", 2)
> > +		    && strcmp(argv[i], "-isystem"))
> >  			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.
> > +		/* We handle two cases: first the case where -I/-L/-isystem
> > +		 * 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.
> > +		 * -isystem is necessarily in the first case.
> 
>  Unfortunately, that's not true. You can pass something like -isystemfoo and it
> will add ./foo to the search path. Ain't gcc fun... I'll leave it as an exercise
> to the reader to handle that case :-P

I think we should not care too much about this, should we? In the end,
this would not be an unsafe path...

But I think my next iteration should cover all your comments (as well as
on the previous patch).

Thanks! ;-)

Regards,
Yann E. MORIN.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem
  2016-08-24 14:12     ` Yann E. MORIN
@ 2016-08-24 15:23       ` Arnout Vandecappelle
  0 siblings, 0 replies; 7+ messages in thread
From: Arnout Vandecappelle @ 2016-08-24 15:23 UTC (permalink / raw)
  To: buildroot



On 24-08-16 16:12, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2016-08-24 03:18 +0200, Arnout Vandecappelle spake thusly:
>> On 17-08-16 16:42, Yann E. MORIN wrote:
>>> 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 fo -I and -L, so
>>> the paranoid check does not kick in.
>>>
>>> Extend the paranoid check to also look for the -isystem option.
>>
>>  While we're at it: -idirafter, -iprefix, -iwithprefix, -iwithprefixbefore,
>> -isysroot, -imultilib, -iquote.
> 
> Did you meant we should handle all of them now? Are were you listing
> them for the future, when we encoutner issues with any if them?

 I think it doesn't hurt to include these now. Though the prefix ones are a bit
iffy (you could pass -iprefix /usr/ -iwithprefix lib and this wouldn't be
captured by the paranoid check). But certainly -idirafter and -iquote should be
handled now IMHO.

> 
>>  And then there is -B, but if someone passes that, it's really broken :-) And
>> --sysroot, also interesting if that is passed. But I guess these things are
>> going a bit too far.
> 
> --sysroot is even more fun, as it can be written: --sysroot=dir , so
> we'd need to take care of this as well...

 Well, anything that is passing --sysroot is doing really crazy shit so chances
are our paranoid check is wrong anyway, so let's ignore that one.

> 
> Lotta fun in sight! ;-)
> 
>>> 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>
>>> ---
>>>  toolchain/toolchain-wrapper.c | 18 ++++++++++--------
>>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
>>> index b8b3cbe..8a9c3b3 100644
>>> --- a/toolchain/toolchain-wrapper.c
>>> +++ b/toolchain/toolchain-wrapper.c
>>> @@ -241,17 +241,19 @@ 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))
>>> +		/* Skip options that do not start with -I, -isystem or -L */
>>> +		if (   strncmp(argv[i], "-I", 2)
>>> +		    && strncmp(argv[i], "-L", 2)
>>> +		    && strcmp(argv[i], "-isystem"))
>>>  			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.
>>> +		/* We handle two cases: first the case where -I/-L/-isystem
>>> +		 * 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.
>>> +		 * -isystem is necessarily in the first case.
>>
>>  Unfortunately, that's not true. You can pass something like -isystemfoo and it
>> will add ./foo to the search path. Ain't gcc fun... I'll leave it as an exercise
>> to the reader to handle that case :-P
> 
> I think we should not care too much about this, should we? In the end,
> this would not be an unsafe path...

 I gave -isystemfoo as an example because it looks so funny. but
-isystem/usr/lib is possible as well.

 Fortunately, your v2 handles that!

 Regards,
 Arnout

> 
> But I think my next iteration should cover all your comments (as well as
> on the previous patch).
> 
> Thanks! ;-)
> 
> Regards,
> Yann E. MORIN.
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem
  2016-08-24 14:19 Yann E. MORIN
@ 2016-08-24 14:19 ` Yann E. MORIN
  0 siblings, 0 replies; 7+ messages in thread
From: Yann E. MORIN @ 2016-08-24 14:19 UTC (permalink / raw)
  To: buildroot

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 -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 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 | 47 +++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index edade43..caf62e7 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -80,6 +80,20 @@ static char *predef_args[] = {
 #endif
 };
 
+struct unsafe_opt_s {
+	const char *arg;
+	size_t     len;
+};
+
+/* 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(-isystem),
+	UNSAFE_OPT(-L),
+	{ NULL, 0 },
+};
+
 static void check_unsafe_path(const char *arg,
 			      const char *path,
 			      int paranoid,
@@ -233,24 +247,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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-08-24 15:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-17 14:42 [Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure Yann E. MORIN
2016-08-17 14:42 ` [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem Yann E. MORIN
2016-08-24  1:18   ` Arnout Vandecappelle
2016-08-24 14:12     ` Yann E. MORIN
2016-08-24 15:23       ` Arnout Vandecappelle
2016-08-24  1:09 ` [Buildroot] [PATCH 1/2] toolchain/wrapper: display options leading to a paranoid failure Arnout Vandecappelle
2016-08-24 14:19 Yann E. MORIN
2016-08-24 14:19 ` [Buildroot] [PATCH 2/2] toolchain/wrapper: extend paranoid check to -isystem Yann E. MORIN

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.