| Message ID | 1471444962-32200-2-git-send-email-yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
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, 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.
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. >
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;
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(-)