| Message ID | 1392074881-12508-1-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
Thomas, All, On 2014-02-11 00:28 +0100, Thomas Petazzoni spake thusly: > The CodeSourcery toolchains have a very interesting feature: they warn > the user when an unsafe header or library path is used, i.e a path > that will lead host headers or libraries to leak into the build. > > This commit adds a similar functionality into our external toolchain > wrapper, so that it can be used with all external toolchains, and can > also be tuned as needed. By default, the external toolchain wrapper > now gives warnings such as: > > WARNING: unsafe header/library path used in cross-compilation: '-I /usr/foo' > WARNING: unsafe header/library path used in cross-compilation: '-L /usr/bleh' > > but the compilation continues successfully. One can then easily grep > in his build log to search for occurences of this message. > > Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the > external wrapper will instead error out and abort the compilation. We > could then one day imagine setting this BR_PARANOID_WRAPPER in the > autobuilders. I think I'd prefer we do as for BR_DEBUG_WRAPPER: BR2_PARANOID_WRAPPER undefined or empty: nothing BR2_PARANOID_WRAPPER=WARNING : warning BR2_PARANOID_WRAPPER=ERROR : error out (or use numbers if you prefer) Side-note: BR_DEBUG_WRAPPER should probably be renamed to BR2_DEBUG_WRAPPER, to follow the newly-stated naming scheme, no? > A similar change could be made to the internal toolchain backend > either by making it use a wrapper like the external toolchain one, or > by adding some patches to gcc, by borrowing the changes made by the > CodeSourcery people. Maybe it would be best to not duplicate this, and always use the wrapper, even for the internal backend? > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > .../toolchain-external/ext-toolchain-wrapper.c | 27 ++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c > index 81c6ed1..c8dcad5 100644 > --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c > +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c > @@ -77,6 +77,7 @@ int main(int argc, char **argv) > char *progpath = argv[0]; > char *basename; > char *env_debug; > + char *paranoid_wrapper; > int ret, i, count = 0, debug; > > /* Calculate the relative paths */ > @@ -178,6 +179,32 @@ int main(int argc, char **argv) > } > #endif /* ARCH || TUNE || CPU */ > > + paranoid_wrapper = getenv("BR_PARANOID_WRAPPER"); > + > + /* Check for unsafe library and header paths */ > + for (i = 1; i < argc; i++) { > + if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) || > + !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) { No need for strncmp: you're comparing to a constant, so it will not overflow. Also, using strlen on an argument of strncmp is flawed to begin with. For strncmp(a, b, strlen(b)) : - if 'b' is a constant, there's no need for strncmp to begin with - if 'b' is a user-supplied value, strlen will happily go west, and will not protect strncmp anyway. > + fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s'\n", > + paranoid_wrapper ? "ERROR" : "WARNING", argv[i]); > + if (paranoid_wrapper) > + exit(1); > + continue; > + } > + > + if (!strcmp(argv[i], "-L") || !strcmp(argv[i], "-I")) { And here you're using strcmp. > + if (i == argc) > + continue; > + if (!strncmp(argv[i+1], "/usr", strlen("/usr"))) { Ditto strncmp. > + fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s %s'\n", > + paranoid_wrapper ? "ERROR" : "WARNING", argv[i], argv[i + 1]); > + if (paranoid_wrapper) > + exit(1); > + continue; > + } > + } > + } > + > /* append forward args */ > memcpy(cur, &argv[1], sizeof(char *) * (argc - 1)); > cur += argc - 1; Otherwise, I like the idea pretty much! :-) Regards, Yann E. MORIN.
Hi Thomas, On Tue, Feb 11, 2014 at 12:28:01AM +0100, Thomas Petazzoni wrote: > The CodeSourcery toolchains have a very interesting feature: they warn > the user when an unsafe header or library path is used, i.e a path > that will lead host headers or libraries to leak into the build. > > This commit adds a similar functionality into our external toolchain > wrapper, so that it can be used with all external toolchains, and can > also be tuned as needed. By default, the external toolchain wrapper > now gives warnings such as: > > WARNING: unsafe header/library path used in cross-compilation: '-I /usr/foo' > WARNING: unsafe header/library path used in cross-compilation: '-L /usr/bleh' I'd mention that this makes Buildroot builds under /usr even more problematic. I thought this limitation appears in the documentation, but I can't find it there now. baruch
Dear Yann E. MORIN, On Tue, 11 Feb 2014 01:33:27 +0100, Yann E. MORIN wrote: > > Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the > > external wrapper will instead error out and abort the compilation. We > > could then one day imagine setting this BR_PARANOID_WRAPPER in the > > autobuilders. > > I think I'd prefer we do as for BR_DEBUG_WRAPPER: > BR2_PARANOID_WRAPPER undefined or empty: nothing > BR2_PARANOID_WRAPPER=WARNING : warning > BR2_PARANOID_WRAPPER=ERROR : error out Sure, I don't have any particular feeling about this. However, I would maybe like to have this feature enabled as the warning level by default. > Side-note: BR_DEBUG_WRAPPER should probably be renamed to > BR2_DEBUG_WRAPPER, to follow the newly-stated naming scheme, no? If I understood the newly-stated naming scheme, yes, I believe you're right, it should be renamed BR2_DEBUG_WRAPPER. > > A similar change could be made to the internal toolchain backend > > either by making it use a wrapper like the external toolchain one, or > > by adding some patches to gcc, by borrowing the changes made by the > > CodeSourcery people. > > Maybe it would be best to not duplicate this, and always use the > wrapper, even for the internal backend? Which is exactly one of the two solutions I proposed in the paragraph you're quoting :-) > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > --- > > .../toolchain-external/ext-toolchain-wrapper.c | 27 ++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c > > index 81c6ed1..c8dcad5 100644 > > --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c > > +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c > > @@ -77,6 +77,7 @@ int main(int argc, char **argv) > > char *progpath = argv[0]; > > char *basename; > > char *env_debug; > > + char *paranoid_wrapper; > > int ret, i, count = 0, debug; > > > > /* Calculate the relative paths */ > > @@ -178,6 +179,32 @@ int main(int argc, char **argv) > > } > > #endif /* ARCH || TUNE || CPU */ > > > > + paranoid_wrapper = getenv("BR_PARANOID_WRAPPER"); > > + > > + /* Check for unsafe library and header paths */ > > + for (i = 1; i < argc; i++) { > > + if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) || > > + !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) { > > No need for strncmp: you're comparing to a constant, so it will not > overflow. I need strncmp here: I want to match only the first characters of argv[i]. I.e, the above condition should match: -I/usr/bar -L/usr/local/foo -I/usr/include/mysql etc. If I use strcmp(), it is going to compare the entire string, and -I/usr/include/mysql is not the same as -I/usr. > Also, using strlen on an argument of strncmp is flawed to begin with. Weird, a certain Yann E. Morin did exactly this in http://git.buildroot.net/buildroot/commit/toolchain/toolchain-external/ext-toolchain-wrapper.c?id=2c1dc32647eb308126b0ae80a91988059d39aa7b :-) > For strncmp(a, b, strlen(b)) : > - if 'b' is a constant, there's no need for strncmp to begin with > - if 'b' is a user-supplied value, strlen will happily go west, and > will not protect strncmp anyway. You miss the case where you use strncmp() to only match the first N characters of the string. Which is both what I'm doing here, and what your patch for -march, -mtune and -mcpu is doing. > > + fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s'\n", > > + paranoid_wrapper ? "ERROR" : "WARNING", argv[i]); > > + if (paranoid_wrapper) > > + exit(1); > > + continue; > > + } > > + > > + if (!strcmp(argv[i], "-L") || !strcmp(argv[i], "-I")) { > > And here you're using strcmp. Yes, because I'm matching the entire string. > > > + if (i == argc) > > + continue; > > + if (!strncmp(argv[i+1], "/usr", strlen("/usr"))) { > > Ditto strncmp. Yes because I want to match only the beginning of the string, so that this condition together with the above matches: -I /usr/include/mysql -L /usr/foo > Otherwise, I like the idea pretty much! :-) Me too :) Best regards, Thomas
Dear Baruch Siach, On Tue, 11 Feb 2014 08:21:40 +0200, Baruch Siach wrote: > On Tue, Feb 11, 2014 at 12:28:01AM +0100, Thomas Petazzoni wrote: > > The CodeSourcery toolchains have a very interesting feature: they warn > > the user when an unsafe header or library path is used, i.e a path > > that will lead host headers or libraries to leak into the build. > > > > This commit adds a similar functionality into our external toolchain > > wrapper, so that it can be used with all external toolchains, and can > > also be tuned as needed. By default, the external toolchain wrapper > > now gives warnings such as: > > > > WARNING: unsafe header/library path used in cross-compilation: '-I /usr/foo' > > WARNING: unsafe header/library path used in cross-compilation: '-L /usr/bleh' > > I'd mention that this makes Buildroot builds under /usr even more problematic. Yes, this is true. Technically speaking, testing for -I/usr or -L/usr is not the ideal way to achieve this. The ideal way would be to look if only headers/libraries from the toolchain sysroot, and from the package source tree are used. But this is fairly hard to achieve, unfortunately. I am open to suggestions on how to achieve this. But in any case, this mechanism will have to have a mechanism to be entirely disabled. > I thought this limitation appears in the documentation, but I can't find it > there now. I don't think it's written in the documentation, but we have a bug report for it, at https://bugs.busybox.net/show_bug.cgi?id=5750. Best regards, Thomas
Thomas, All, On 2014-02-11 09:18 +0100, Thomas Petazzoni spake thusly: > On Tue, 11 Feb 2014 01:33:27 +0100, Yann E. MORIN wrote: > > > > Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the > > > external wrapper will instead error out and abort the compilation. We > > > could then one day imagine setting this BR_PARANOID_WRAPPER in the > > > autobuilders. > > > > I think I'd prefer we do as for BR_DEBUG_WRAPPER: > > BR2_PARANOID_WRAPPER undefined or empty: nothing > > BR2_PARANOID_WRAPPER=WARNING : warning > > BR2_PARANOID_WRAPPER=ERROR : error out > > Sure, I don't have any particular feeling about this. However, I would > maybe like to have this feature enabled as the warning level by default. Probably sane, indeed. > > Side-note: BR_DEBUG_WRAPPER should probably be renamed to > > BR2_DEBUG_WRAPPER, to follow the newly-stated naming scheme, no? > > If I understood the newly-stated naming scheme, yes, I believe you're > right, it should be renamed BR2_DEBUG_WRAPPER. Ok, will cook a patch. > > > A similar change could be made to the internal toolchain backend > > > either by making it use a wrapper like the external toolchain one, or > > > by adding some patches to gcc, by borrowing the changes made by the > > > CodeSourcery people. > > > > Maybe it would be best to not duplicate this, and always use the > > wrapper, even for the internal backend? > > Which is exactly one of the two solutions I proposed in the paragraph > you're quoting :-) And I was just expressing my preference. ;-) > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > > --- > > > .../toolchain-external/ext-toolchain-wrapper.c | 27 ++++++++++++++++++++++ > > > 1 file changed, 27 insertions(+) > > > > > > diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c > > > index 81c6ed1..c8dcad5 100644 > > > --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c > > > +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c > > > @@ -77,6 +77,7 @@ int main(int argc, char **argv) > > > char *progpath = argv[0]; > > > char *basename; > > > char *env_debug; > > > + char *paranoid_wrapper; > > > int ret, i, count = 0, debug; > > > > > > /* Calculate the relative paths */ > > > @@ -178,6 +179,32 @@ int main(int argc, char **argv) > > > } > > > #endif /* ARCH || TUNE || CPU */ > > > > > > + paranoid_wrapper = getenv("BR_PARANOID_WRAPPER"); > > > + > > > + /* Check for unsafe library and header paths */ > > > + for (i = 1; i < argc; i++) { > > > + if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) || > > > + !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) { Don't we also want to check -L/lib as well? > > No need for strncmp: you're comparing to a constant, so it will not > > overflow. > > I need strncmp here: I want to match only the first characters of > argv[i]. I.e, the above condition should match: > > -I/usr/bar > -L/usr/local/foo > -I/usr/include/mysql > > etc. If I use strcmp(), it is going to compare the entire string, and > -I/usr/include/mysql is not the same as -I/usr. Doh, righto-right. I should not review patches so late in the night... > > Also, using strlen on an argument of strncmp is flawed to begin with. > > Weird, a certain Yann E. Morin did exactly this in > http://git.buildroot.net/buildroot/commit/toolchain/toolchain-external/ext-toolchain-wrapper.c?id=2c1dc32647eb308126b0ae80a91988059d39aa7b :-) Hehe! ;-) Regards, Yann E. MORIN.
diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c index 81c6ed1..c8dcad5 100644 --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c @@ -77,6 +77,7 @@ int main(int argc, char **argv) char *progpath = argv[0]; char *basename; char *env_debug; + char *paranoid_wrapper; int ret, i, count = 0, debug; /* Calculate the relative paths */ @@ -178,6 +179,32 @@ int main(int argc, char **argv) } #endif /* ARCH || TUNE || CPU */ + paranoid_wrapper = getenv("BR_PARANOID_WRAPPER"); + + /* Check for unsafe library and header paths */ + for (i = 1; i < argc; i++) { + if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) || + !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) { + fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s'\n", + paranoid_wrapper ? "ERROR" : "WARNING", argv[i]); + if (paranoid_wrapper) + exit(1); + continue; + } + + if (!strcmp(argv[i], "-L") || !strcmp(argv[i], "-I")) { + if (i == argc) + continue; + if (!strncmp(argv[i+1], "/usr", strlen("/usr"))) { + fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s %s'\n", + paranoid_wrapper ? "ERROR" : "WARNING", argv[i], argv[i + 1]); + if (paranoid_wrapper) + exit(1); + continue; + } + } + } + /* append forward args */ memcpy(cur, &argv[1], sizeof(char *) * (argc - 1)); cur += argc - 1;
The CodeSourcery toolchains have a very interesting feature: they warn the user when an unsafe header or library path is used, i.e a path that will lead host headers or libraries to leak into the build. This commit adds a similar functionality into our external toolchain wrapper, so that it can be used with all external toolchains, and can also be tuned as needed. By default, the external toolchain wrapper now gives warnings such as: WARNING: unsafe header/library path used in cross-compilation: '-I /usr/foo' WARNING: unsafe header/library path used in cross-compilation: '-L /usr/bleh' but the compilation continues successfully. One can then easily grep in his build log to search for occurences of this message. Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the external wrapper will instead error out and abort the compilation. We could then one day imagine setting this BR_PARANOID_WRAPPER in the autobuilders. A similar change could be made to the internal toolchain backend either by making it use a wrapper like the external toolchain one, or by adding some patches to gcc, by borrowing the changes made by the CodeSourcery people. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- .../toolchain-external/ext-toolchain-wrapper.c | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+)