| Message ID | 1442685931-20556-1-git-send-email-brendanheading@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 19 September 2015 at 19:05, Brendan Heading <brendanheading@gmail.com> wrote: > Fixes: > http://autobuild.buildroot.net/results/123/123a5b3f72ba8c1a4aa1cea5b7b846a04fd4e923/ > http://autobuild.buildroot.net/results/38c/38cfa4e7249a8770b06dbd392acba79303d3f9bc/ > > .. and others. > > Improve GCC's checking of stack smashing support, adding a specific case > statement for uclibc, and removing the check done after checking the > glibc version. > > Signed-off-by: Brendan Heading <brendanheading@gmail.com> > --- > Patch V1 - this initial version only fixes 4.7.4 - wanted to check that > we are heading the right way before I fix all the other versions. > > Note that I manually modified configure, rather than regenerating it from > configure.ac. > > Original plan was to reverse the order of the existing __GLIBC_MINOR__ and > uclibc check. However, the uclibc check falls through if it does not > detect uclibc, so I figure it better to add the separate case statement. Hi guys, Just a quick ping to draw your attention to the above patch. Before I add the fix to all the other supported versions of GCC I wanted to check that you are happy with the way I'm solving the problem in 4.7.4. Brendan
On 19-09-15 20:05, Brendan Heading wrote: > Fixes: > http://autobuild.buildroot.net/results/123/123a5b3f72ba8c1a4aa1cea5b7b846a04fd4e923/ > http://autobuild.buildroot.net/results/38c/38cfa4e7249a8770b06dbd392acba79303d3f9bc/ > > .. and others. > > Improve GCC's checking of stack smashing support, adding a specific case > statement for uclibc, and removing the check done after checking the > glibc version. > > Signed-off-by: Brendan Heading <brendanheading@gmail.com> > --- > Patch V1 - this initial version only fixes 4.7.4 - wanted to check that > we are heading the right way before I fix all the other versions. > > Note that I manually modified configure, rather than regenerating it from > configure.ac. I think it would be a good idea to send this patch upstream in parallel to get feedback from there as well. But then you should make it against 5.2.0 of course. > Original plan was to reverse the order of the existing __GLIBC_MINOR__ and > uclibc check. However, the uclibc check falls through if it does not > detect uclibc, so I figure it better to add the separate case statement. IMHO it's better to leave this kind of comment in the commit log itself (i.e. before the ---). Then there is an easy trace of it if someone, years down the line, wonders why it was not done that way. Also, it would be possible to move the condition up by just splitting it: if __UCLIBC__; then if __UCLIBC_HAS_SSP__; then yes else no elif __GLIBC__ ... > --- > ...ing-of-stack-smashing-support-with-uclibc.patch | 81 ++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 package/gcc/4.7.4/920-gcc-improve-checking-of-stack-smashing-support-with-uclibc.patch > > diff --git a/package/gcc/4.7.4/920-gcc-improve-checking-of-stack-smashing-support-with-uclibc.patch b/package/gcc/4.7.4/920-gcc-improve-checking-of-stack-smashing-support-with-uclibc.patch > new file mode 100644 > index 0000000..8f5d961 > --- /dev/null > +++ b/package/gcc/4.7.4/920-gcc-improve-checking-of-stack-smashing-support-with-uclibc.patch > @@ -0,0 +1,81 @@ > +From 39bbd72364c34da7ba78f26354512eb5229cebdc Mon Sep 17 00:00:00 2001 > +From: Brendan Heading <brendanheading@gmail.com> > +Date: Fri, 18 Sep 2015 14:54:25 +0100 > +Subject: [PATCH 1/1] gcc: improve checking of stack smashing support with Please avoid the 1/1 bit, by adding -N to format-patch. > + uclibc > + > +Signed-off-by: Brendan Heading <brendanheading@gmail.com> > +--- > + gcc/configure | 15 +++++++++------ > + gcc/configure.ac | 15 +++++++++------ > + 2 files changed, 18 insertions(+), 12 deletions(-) > + > +diff --git a/gcc/configure b/gcc/configure > +index 63cba0a..d1ac2ea 100755 > +--- a/gcc/configure > ++++ b/gcc/configure > +@@ -26797,6 +26797,15 @@ else > + *-*-musl*) > + # All versions of musl provide stack protector > + gcc_cv_libc_provides_ssp=yes;; > ++ *-*-uclibc*) Can we be sure that uclibc will always have this in the tuple? Well, _we_ can be sure of course, but can upstream be sure? > ++ if $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC__[ ]+1' \ > ++ $target_header_dir/features.h > /dev/null && \ You didn't check the existence of features.h (like is done below). Regards, Arnout > ++ test -f $target_header_dir/bits/uClibc_config.h && \ > ++ $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC_HAS_SSP__[ ]+1' \ > ++ $target_header_dir/bits/uClibc_config.h > /dev/null; then > ++ gcc_cv_libc_provides_ssp=yes > ++ fi > ++ ;; > + *-*-linux* | *-*-kfreebsd*-gnu | *-*-knetbsd*-gnu) > + # glibc 2.4 and later provides __stack_chk_fail and > + # either __stack_chk_guard, or TLS access to stack guard canary. > +@@ -26811,12 +26820,6 @@ else > + && $EGREP '^[ ]*#[ ]*define[ ]+__GLIBC_MINOR__[ ]+([1-9][0-9]|[4-9])' \ > + $target_header_dir/features.h > /dev/null; then > + gcc_cv_libc_provides_ssp=yes > +- elif $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC__[ ]+1' \ > +- $target_header_dir/features.h > /dev/null && \ > +- test -f $target_header_dir/bits/uClibc_config.h && \ > +- $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC_HAS_SSP__[ ]+1' \ > +- $target_header_dir/bits/uClibc_config.h > /dev/null; then > +- gcc_cv_libc_provides_ssp=yes > + fi > + # all versions of Bionic support stack protector > + elif test -f $target_header_dir/sys/cdefs.h \ > +diff --git a/gcc/configure.ac b/gcc/configure.ac > +index ea1c147..5954cc7 100644 > +--- a/gcc/configure.ac > ++++ b/gcc/configure.ac > +@@ -4672,6 +4672,15 @@ AC_CACHE_CHECK(__stack_chk_fail in target C library, > + *-*-musl*) > + # All versions of musl provide stack protector > + gcc_cv_libc_provides_ssp=yes;; > ++ *-*-uclibc*) > ++ if $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC__[ ]+1' \ > ++ $target_header_dir/features.h > /dev/null && \ > ++ test -f $target_header_dir/bits/uClibc_config.h && \ > ++ $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC_HAS_SSP__[ ]+1' \ > ++ $target_header_dir/bits/uClibc_config.h > /dev/null; then > ++ gcc_cv_libc_provides_ssp=yes > ++ fi > ++ ;; > + *-*-linux* | *-*-kfreebsd*-gnu | *-*-knetbsd*-gnu) > + [# glibc 2.4 and later provides __stack_chk_fail and > + # either __stack_chk_guard, or TLS access to stack guard canary. > +@@ -4686,12 +4695,6 @@ AC_CACHE_CHECK(__stack_chk_fail in target C library, > + && $EGREP '^[ ]*#[ ]*define[ ]+__GLIBC_MINOR__[ ]+([1-9][0-9]|[4-9])' \ > + $target_header_dir/features.h > /dev/null; then > + gcc_cv_libc_provides_ssp=yes > +- elif $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC__[ ]+1' \ > +- $target_header_dir/features.h > /dev/null && \ > +- test -f $target_header_dir/bits/uClibc_config.h && \ > +- $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC_HAS_SSP__[ ]+1' \ > +- $target_header_dir/bits/uClibc_config.h > /dev/null; then > +- gcc_cv_libc_provides_ssp=yes > + fi > + # all versions of Bionic support stack protector > + elif test -f $target_header_dir/sys/cdefs.h \ > +-- > +2.4.3 > + >
Hi Arnout, sorry for the delay in replying. >> Note that I manually modified configure, rather than regenerating it from >> configure.ac. > > I think it would be a good idea to send this patch upstream in parallel to get > feedback from there as well. But then you should make it against 5.2.0 of course. Happy to follow your guidance on this (and your other notes) but first I should explain my rationale. There's most likely zero chance of getting patches accepted against any of the GCC releases which are in maintenance. That means all of the releases which are currently supported by buildroot. We *might* get patches accepted against mainline, but Thomas suggested that it's hard to get patches into GCC in any case. Either way, I have a feeling that it might not be straightforward to backport patches from GCC 6.0 (the next release) all the way back to our oldest supported release which is 4.7.x. So my approach has been to aim at building a set of patches knowing that they are not likely to be integrated, meaning we can focus on keeping the maintenance as simple as possible on the buildroot side. Separately, I can start the conversation over on the GCC list about improving it in the future. If you look at the GCC configure script, you will see that its last-ditch fallback is to try to detect the presence of the stack smashing helper functions. I don't understand why they don't just use this check by itself as it should work 100% reliably in all possible cases. In terms of lines of code it's an invasive change to move to that way of working, so my idea is to propose that for the upstream head, and the simpler approach for the older releases here. >> Original plan was to reverse the order of the existing __GLIBC_MINOR__ and >> uclibc check. However, the uclibc check falls through if it does not >> detect uclibc, so I figure it better to add the separate case statement. > > IMHO it's better to leave this kind of comment in the commit log itself (i.e. > before the ---). Then there is an easy trace of it if someone, years down the > line, wonders why it was not done that way. Sure. > Also, it would be possible to move the condition up by just splitting it: > > if __UCLIBC__; then > if __UCLIBC_HAS_SSP__; then > yes > else > no > elif __GLIBC__ ... Looks reasonable. I will revise my patch do do this. > Please avoid the 1/1 bit, by adding -N to format-patch. OK. >> + # All versions of musl provide stack protector >> + gcc_cv_libc_provides_ssp=yes;; >> ++ *-*-uclibc*) > > Can we be sure that uclibc will always have this in the tuple? Well, _we_ can > be sure of course, but can upstream be sure? No, we can't be sure as you note. I think this way of checking it is pretty poor. And I don't really understand why they even do all this. If you look further down you will see that the default case is to perform a check for the presence of the stack-smashing helper functions. I'll resubmit the patch with your suggestions accounted for. What do you think of the idea of doing a wholesale change that gets rid of the entire check and simply uses the check for the helper functions ? Brendan
On 28-09-15 15:04, Brendan Heading wrote: > Hi Arnout, sorry for the delay in replying. > >>> Note that I manually modified configure, rather than regenerating it from >>> configure.ac. >> >> I think it would be a good idea to send this patch upstream in parallel to get >> feedback from there as well. But then you should make it against 5.2.0 of course. > > Happy to follow your guidance on this (and your other notes) but first > I should explain my rationale. > > There's most likely zero chance of getting patches accepted against > any of the GCC releases which are in maintenance. That means all of > the releases which are currently supported by buildroot. We *might* > get patches accepted against mainline, but Thomas suggested that it's > hard to get patches into GCC in any case. I don't think that that's what he told you. He just told you that you need to do copyright assignment for non-trivial changes, but your changes is probably considered trivial, and anyway you may not have a problem with doing copyright assignment. > Either way, I have a feeling > that it might not be straightforward to backport patches from GCC 6.0 > (the next release) all the way back to our oldest supported release > which is 4.7.x. I don't agree at all. It's true that that piece of code has changed in current master, but at least the principle still applies. I.e., in which order things are checked. > So my approach has been to aim at building a set of patches knowing > that they are not likely to be integrated, meaning we can focus on > keeping the maintenance as simple as possible on the buildroot side. Well, no, diverging from upstream makes maintenance harder, not simpler. > Separately, I can start the conversation over on the GCC list about > improving it in the future. Separately is OK, as long as the upstream conversation is started in parallel. > If you look at the GCC configure script, you will see that its > last-ditch fallback is to try to detect the presence of the stack > smashing helper functions. I don't understand why they don't just use > this check by itself as it should work 100% reliably in all possible > cases. In terms of lines of code it's an invasive change to move to > that way of working, so my idea is to propose that for the upstream > head, and the simpler approach for the older releases here. Well, since as you say the check for the existence of the functions is more reliable, I would prefer to have that one for the old gcc versions as well. >>> Original plan was to reverse the order of the existing __GLIBC_MINOR__ and >>> uclibc check. However, the uclibc check falls through if it does not >>> detect uclibc, so I figure it better to add the separate case statement. >> >> IMHO it's better to leave this kind of comment in the commit log itself (i.e. >> before the ---). Then there is an easy trace of it if someone, years down the >> line, wonders why it was not done that way. > > Sure. > >> Also, it would be possible to move the condition up by just splitting it: >> >> if __UCLIBC__; then >> if __UCLIBC_HAS_SSP__; then >> yes >> else >> no >> elif __GLIBC__ ... > > Looks reasonable. I will revise my patch do do this. > >> Please avoid the 1/1 bit, by adding -N to format-patch. > > OK. It seems you forgot to do that in v2. > >>> + # All versions of musl provide stack protector >>> + gcc_cv_libc_provides_ssp=yes;; >>> ++ *-*-uclibc*) >> >> Can we be sure that uclibc will always have this in the tuple? Well, _we_ can >> be sure of course, but can upstream be sure? > > No, we can't be sure as you note. I think this way of checking it is > pretty poor. > > And I don't really understand why they even do all this. If you look > further down you will see that the default case is to perform a check > for the presence of the stack-smashing helper functions. > > I'll resubmit the patch with your suggestions accounted for. > > What do you think of the idea of doing a wholesale change that gets > rid of the entire check and simply uses the check for the helper > functions ? As mentioned above, I think that that is the better approach, but it's really worthwhile to discuss this upstream as well. Regards, Arnout
>> So my approach has been to aim at building a set of patches knowing >> that they are not likely to be integrated, meaning we can focus on >> keeping the maintenance as simple as possible on the buildroot side. > > Well, no, diverging from upstream makes maintenance harder, not simpler. I don't see how we have a choice when it comes to older GCC releases. Making a relatively big change to trunk and backporting it is going to be a more invasive change in this case. But it's academic, as I think we agree in any case that the more comprehensive approach (ie doing the detection of symbols only and getting rid of all the other checks) might be best in the long run. >> What do you think of the idea of doing a wholesale change that gets >> rid of the entire check and simply uses the check for the helper >> functions ? > > As mentioned above, I think that that is the better approach, but it's really > worthwhile to discuss this upstream as well. Okay. I will do that next before submitting any further patches here. regards Brendan
Hi Waldemar, Not yet Waldemar .. I started a new job recently and I need to have a word with them before I submit any more patches. Hope to have updates soon .. regards Brendan On 8 October 2015 at 19:13, Waldemar Brodkorb <mail@waldemar-brodkorb.de> wrote: > Hi Brendan, > > any news about this? > > Brendan Heading wrote, > >> On 19 September 2015 at 19:05, Brendan Heading <brendanheading@gmail.com> wrote: >> > Fixes: >> > http://autobuild.buildroot.net/results/123/123a5b3f72ba8c1a4aa1cea5b7b846a04fd4e923/ >> > http://autobuild.buildroot.net/results/38c/38cfa4e7249a8770b06dbd392acba79303d3f9bc/ >> > >> > .. and others. >> > >> > Improve GCC's checking of stack smashing support, adding a specific case >> > statement for uclibc, and removing the check done after checking the >> > glibc version. >> > >> > Signed-off-by: Brendan Heading <brendanheading@gmail.com> >> > --- >> > Patch V1 - this initial version only fixes 4.7.4 - wanted to check that >> > we are heading the right way before I fix all the other versions. >> > >> > Note that I manually modified configure, rather than regenerating it from >> > configure.ac. >> > >> > Original plan was to reverse the order of the existing __GLIBC_MINOR__ and >> > uclibc check. However, the uclibc check falls through if it does not >> > detect uclibc, so I figure it better to add the separate case statement. >> >> Hi guys, >> >> Just a quick ping to draw your attention to the above patch. Before I >> add the fix to all the other supported versions of GCC I wanted to >> check that you are happy with the way I'm solving the problem in >> 4.7.4. >> >> Brendan >>
diff --git a/package/gcc/4.7.4/920-gcc-improve-checking-of-stack-smashing-support-with-uclibc.patch b/package/gcc/4.7.4/920-gcc-improve-checking-of-stack-smashing-support-with-uclibc.patch new file mode 100644 index 0000000..8f5d961 --- /dev/null +++ b/package/gcc/4.7.4/920-gcc-improve-checking-of-stack-smashing-support-with-uclibc.patch @@ -0,0 +1,81 @@ +From 39bbd72364c34da7ba78f26354512eb5229cebdc Mon Sep 17 00:00:00 2001 +From: Brendan Heading <brendanheading@gmail.com> +Date: Fri, 18 Sep 2015 14:54:25 +0100 +Subject: [PATCH 1/1] gcc: improve checking of stack smashing support with + uclibc + +Signed-off-by: Brendan Heading <brendanheading@gmail.com> +--- + gcc/configure | 15 +++++++++------ + gcc/configure.ac | 15 +++++++++------ + 2 files changed, 18 insertions(+), 12 deletions(-) + +diff --git a/gcc/configure b/gcc/configure +index 63cba0a..d1ac2ea 100755 +--- a/gcc/configure ++++ b/gcc/configure +@@ -26797,6 +26797,15 @@ else + *-*-musl*) + # All versions of musl provide stack protector + gcc_cv_libc_provides_ssp=yes;; ++ *-*-uclibc*) ++ if $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC__[ ]+1' \ ++ $target_header_dir/features.h > /dev/null && \ ++ test -f $target_header_dir/bits/uClibc_config.h && \ ++ $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC_HAS_SSP__[ ]+1' \ ++ $target_header_dir/bits/uClibc_config.h > /dev/null; then ++ gcc_cv_libc_provides_ssp=yes ++ fi ++ ;; + *-*-linux* | *-*-kfreebsd*-gnu | *-*-knetbsd*-gnu) + # glibc 2.4 and later provides __stack_chk_fail and + # either __stack_chk_guard, or TLS access to stack guard canary. +@@ -26811,12 +26820,6 @@ else + && $EGREP '^[ ]*#[ ]*define[ ]+__GLIBC_MINOR__[ ]+([1-9][0-9]|[4-9])' \ + $target_header_dir/features.h > /dev/null; then + gcc_cv_libc_provides_ssp=yes +- elif $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC__[ ]+1' \ +- $target_header_dir/features.h > /dev/null && \ +- test -f $target_header_dir/bits/uClibc_config.h && \ +- $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC_HAS_SSP__[ ]+1' \ +- $target_header_dir/bits/uClibc_config.h > /dev/null; then +- gcc_cv_libc_provides_ssp=yes + fi + # all versions of Bionic support stack protector + elif test -f $target_header_dir/sys/cdefs.h \ +diff --git a/gcc/configure.ac b/gcc/configure.ac +index ea1c147..5954cc7 100644 +--- a/gcc/configure.ac ++++ b/gcc/configure.ac +@@ -4672,6 +4672,15 @@ AC_CACHE_CHECK(__stack_chk_fail in target C library, + *-*-musl*) + # All versions of musl provide stack protector + gcc_cv_libc_provides_ssp=yes;; ++ *-*-uclibc*) ++ if $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC__[ ]+1' \ ++ $target_header_dir/features.h > /dev/null && \ ++ test -f $target_header_dir/bits/uClibc_config.h && \ ++ $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC_HAS_SSP__[ ]+1' \ ++ $target_header_dir/bits/uClibc_config.h > /dev/null; then ++ gcc_cv_libc_provides_ssp=yes ++ fi ++ ;; + *-*-linux* | *-*-kfreebsd*-gnu | *-*-knetbsd*-gnu) + [# glibc 2.4 and later provides __stack_chk_fail and + # either __stack_chk_guard, or TLS access to stack guard canary. +@@ -4686,12 +4695,6 @@ AC_CACHE_CHECK(__stack_chk_fail in target C library, + && $EGREP '^[ ]*#[ ]*define[ ]+__GLIBC_MINOR__[ ]+([1-9][0-9]|[4-9])' \ + $target_header_dir/features.h > /dev/null; then + gcc_cv_libc_provides_ssp=yes +- elif $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC__[ ]+1' \ +- $target_header_dir/features.h > /dev/null && \ +- test -f $target_header_dir/bits/uClibc_config.h && \ +- $EGREP '^[ ]*#[ ]*define[ ]+__UCLIBC_HAS_SSP__[ ]+1' \ +- $target_header_dir/bits/uClibc_config.h > /dev/null; then +- gcc_cv_libc_provides_ssp=yes + fi + # all versions of Bionic support stack protector + elif test -f $target_header_dir/sys/cdefs.h \ +-- +2.4.3 +
Fixes: http://autobuild.buildroot.net/results/123/123a5b3f72ba8c1a4aa1cea5b7b846a04fd4e923/ http://autobuild.buildroot.net/results/38c/38cfa4e7249a8770b06dbd392acba79303d3f9bc/ .. and others. Improve GCC's checking of stack smashing support, adding a specific case statement for uclibc, and removing the check done after checking the glibc version. Signed-off-by: Brendan Heading <brendanheading@gmail.com> --- Patch V1 - this initial version only fixes 4.7.4 - wanted to check that we are heading the right way before I fix all the other versions. Note that I manually modified configure, rather than regenerating it from configure.ac. Original plan was to reverse the order of the existing __GLIBC_MINOR__ and uclibc check. However, the uclibc check falls through if it does not detect uclibc, so I figure it better to add the separate case statement. --- ...ing-of-stack-smashing-support-with-uclibc.patch | 81 ++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 package/gcc/4.7.4/920-gcc-improve-checking-of-stack-smashing-support-with-uclibc.patch