Message ID | 20170317190939.8707-1-vapier@gentoo.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 109250 invoked by alias); 17 Mar 2017 19:09:45 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 108217 invoked by uid 89); 17 Mar 2017 19:09:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.gentoo.org From: Mike Frysinger <vapier@gentoo.org> To: libc-alpha@sourceware.org Subject: [PATCH v2] posix_spawn: use a larger min stack for -fstack-check [BZ #21253] Date: Fri, 17 Mar 2017 12:09:39 -0700 Message-Id: <20170317190939.8707-1-vapier@gentoo.org> In-Reply-To: <20170316073012.22763-1-vapier@gentoo.org> References: <20170316073012.22763-1-vapier@gentoo.org> |
Commit Message
Mike Frysinger
March 17, 2017, 7:09 p.m. UTC
When glibc is built with -fstack-check, trying to use posix_spawn can lead to segfaults due to gcc internally probing stack memory too far. The new spawn API will allocate a minimum of 1 page, but the stack checking logic might probe a couple of pages. When it tries to walk them, everything falls apart. The gcc internal docs [1] state the default interval checking is one page. Which means we need two pages (the current one, and the next probed). No target currently defines it larger. Further, it mentions that the default minimum stack size needed to recover from an overflow is 4/8KiB for sjlj or 8/12KiB for others. But some Linux targets (like mips and ppc) go up to 16KiB (and some non-Linux targets go up to 24KiB). Let's create each child with a minimum of 32KiB slack space to support them all, and give us future breathing room. No test is added as existing ones crash. Even a simple call is enough to trigger the problem: char *argv[] = { "/bin/ls", NULL }; posix_spawn(NULL, "/bin/ls", NULL, NULL, argv, NULL); [1] https://gcc.gnu.org/onlinedocs/gcc-6.3.0/gccint/Stack-Checking.html --- v2 - rework stack padding logic so we always round up to the page size sysdeps/unix/sysv/linux/spawni.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Comments
On 17 Mar 2017 12:09, Mike Frysinger wrote: > - size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize)); > + size_t stack_size = ALIGN_UP (argv_size, GLRO (dl_pagesize)); guess the diff can be a little confusing -- i just fixed spacing here. but for the sake of clarity, i can drop that part too. -mike
On Mär 17 2017, Mike Frysinger <vapier@gentoo.org> wrote: > On 17 Mar 2017 12:09, Mike Frysinger wrote: >> - size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize)); >> + size_t stack_size = ALIGN_UP (argv_size, GLRO (dl_pagesize)); > > guess the diff can be a little confusing -- i just fixed spacing here. The spacing wasn't wrong. Andreas.
On Fri, Mar 17, 2017 at 09:19:24PM +0100, Andreas Schwab wrote: > On Mär 17 2017, Mike Frysinger <vapier@gentoo.org> wrote: > > > On 17 Mar 2017 12:09, Mike Frysinger wrote: > >> - size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize)); > >> + size_t stack_size = ALIGN_UP (argv_size, GLRO (dl_pagesize)); > > > > guess the diff can be a little confusing -- i just fixed spacing here. > > The spacing wasn't wrong. $ git grep 'GLRO (' [^C]* | wc -l 30 $ git grep 'GLRO(' [^C]* | wc -l 524 The tradition is obviously in favour of the latter variant, but why?
On 17 Mar 2017 21:19, Andreas Schwab wrote: > On Mär 17 2017, Mike Frysinger wrote: > > On 17 Mar 2017 12:09, Mike Frysinger wrote: > >> - size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize)); > >> + size_t stack_size = ALIGN_UP (argv_size, GLRO (dl_pagesize)); > > > > guess the diff can be a little confusing -- i just fixed spacing here. > > The spacing wasn't wrong. [citation needed] GNU style says it's wrong: https://www.gnu.org/prep/standards/standards.html#index-spaces-before-open_002dparen -mike
On 17/03/17 21:21, Mike Frysinger wrote: > GNU style says it's wrong: > https://www.gnu.org/prep/standards/standards.html#index-spaces-before-open_002dparen that coding style rule is obviously wrong as it breaks macro definitions, you cannot always put spaces before open-parentheses. in glibc it seems the rule was that there is space in function calls but not necessarily in macro invocation.
On 20 Mar 2017 13:40, Szabolcs Nagy wrote: > On 17/03/17 21:21, Mike Frysinger wrote: > > GNU style says it's wrong: > > https://www.gnu.org/prep/standards/standards.html#index-spaces-before-open_002dparen > > that coding style rule is obviously wrong > as it breaks macro definitions, you cannot > always put spaces before open-parentheses. obviously the style guide isn't going to force something that the language disallows > in glibc it seems the rule was that there > is space in function calls but not necessarily > in macro invocation. unless there's an explicit rule somewhere on the glibc side, then we should be bringing things in line with the style guide. you can't tell people "follow the GNU style guide" and then sometimes be like "oh but not here -- you just have 'to know'". -mike
On 03/20/2017 10:28 AM, Mike Frysinger wrote: > On 20 Mar 2017 13:40, Szabolcs Nagy wrote: >> On 17/03/17 21:21, Mike Frysinger wrote: >>> GNU style says it's wrong: >>> https://www.gnu.org/prep/standards/standards.html#index-spaces-before-open_002dparen >> >> that coding style rule is obviously wrong >> as it breaks macro definitions, you cannot >> always put spaces before open-parentheses. > > obviously the style guide isn't going to force something that the > language disallows > >> in glibc it seems the rule was that there >> is space in function calls but not necessarily >> in macro invocation. > > unless there's an explicit rule somewhere on the glibc side, then > we should be bringing things in line with the style guide. you > can't tell people "follow the GNU style guide" and then sometimes > be like "oh but not here -- you just have 'to know'". I agree. We should follow the GNU style in all possible situations. This hasn't been fixed because nobody has bothered to run a script over everything and submit a patch to clean it all up :-)
On Mon, 20 Mar 2017, Szabolcs Nagy wrote: > in glibc it seems the rule was that there > is space in function calls but not necessarily > in macro invocation. Only certain macros that use concatenation to construct a variable name (GLRO), type name (ElfW) etc. - where the result is conceptually a single identifier. Macros used more like function calls should have the space. That includes ones such as PSEUDO, END, weak_alias, hidden_def etc.
On 20 Mar 2017 16:43, Joseph Myers wrote: > On Mon, 20 Mar 2017, Szabolcs Nagy wrote: > > in glibc it seems the rule was that there > > is space in function calls but not necessarily > > in macro invocation. > > Only certain macros that use concatenation to construct a variable name > (GLRO), type name (ElfW) etc. - where the result is conceptually a single > identifier. Macros used more like function calls should have the space. > That includes ones such as PSEUDO, END, weak_alias, hidden_def etc. do we want to formalize it ? https://sourceware.org/glibc/wiki/Style_and_Conventions#Symbols_and_Parenthesis macros that aren't "functions" should omit the space ? defined() counts as a function right ? -mike
On 17 Mar 2017 12:09, Mike Frysinger wrote: > When glibc is built with -fstack-check, trying to use posix_spawn can > lead to segfaults due to gcc internally probing stack memory too far. > The new spawn API will allocate a minimum of 1 page, but the stack > checking logic might probe a couple of pages. When it tries to walk > them, everything falls apart. > > The gcc internal docs [1] state the default interval checking is one > page. Which means we need two pages (the current one, and the next > probed). No target currently defines it larger. > > Further, it mentions that the default minimum stack size needed to > recover from an overflow is 4/8KiB for sjlj or 8/12KiB for others. > But some Linux targets (like mips and ppc) go up to 16KiB (and some > non-Linux targets go up to 24KiB). > > Let's create each child with a minimum of 32KiB slack space to support > them all, and give us future breathing room. > > No test is added as existing ones crash. Even a simple call is > enough to trigger the problem: > char *argv[] = { "/bin/ls", NULL }; > posix_spawn(NULL, "/bin/ls", NULL, NULL, argv, NULL); ignoring the style change, what do people think of the technical change here ? -mike
On 03/22/2017 02:05 AM, Mike Frysinger wrote: > On 20 Mar 2017 16:43, Joseph Myers wrote: >> On Mon, 20 Mar 2017, Szabolcs Nagy wrote: >>> in glibc it seems the rule was that there >>> is space in function calls but not necessarily >>> in macro invocation. >> >> Only certain macros that use concatenation to construct a variable name >> (GLRO), type name (ElfW) etc. - where the result is conceptually a single >> identifier. Macros used more like function calls should have the space. >> That includes ones such as PSEUDO, END, weak_alias, hidden_def etc. > > do we want to formalize it ? > https://sourceware.org/glibc/wiki/Style_and_Conventions#Symbols_and_Parenthesis > > macros that aren't "functions" should omit the space ? > defined() counts as a function right ? Yes, we should formalize it. Please adjust the wiki. We can always clarify further as time goes on or experience shows otherwise. Yes, non-function macros should omit the space, and defined() in my book counts as a function.
On 03/17/2017 03:09 PM, Mike Frysinger wrote: > When glibc is built with -fstack-check, trying to use posix_spawn can > lead to segfaults due to gcc internally probing stack memory too far. > The new spawn API will allocate a minimum of 1 page, but the stack > checking logic might probe a couple of pages. When it tries to walk > them, everything falls apart. > > The gcc internal docs [1] state the default interval checking is one > page. Which means we need two pages (the current one, and the next > probed). No target currently defines it larger. > > Further, it mentions that the default minimum stack size needed to > recover from an overflow is 4/8KiB for sjlj or 8/12KiB for others. > But some Linux targets (like mips and ppc) go up to 16KiB (and some > non-Linux targets go up to 24KiB). > > Let's create each child with a minimum of 32KiB slack space to support > them all, and give us future breathing room. > > No test is added as existing ones crash. Even a simple call is > enough to trigger the problem: > char *argv[] = { "/bin/ls", NULL }; > posix_spawn(NULL, "/bin/ls", NULL, NULL, argv, NULL); > > [1] https://gcc.gnu.org/onlinedocs/gcc-6.3.0/gccint/Stack-Checking.html > --- > v2 > - rework stack padding logic so we always round up to the page size This looks good to me (modulo the code formatting nit). Overall this kind of internal implementation detail can be changed at any time in the future, so I'm not too worried. Also anyone using posix_spawn enough times to make the VMA space explode with extra pages is using the wrong kind of solution to their problem.
On 22 Mar 2017 12:16, Carlos O'Donell wrote: > On 03/22/2017 02:05 AM, Mike Frysinger wrote: > > On 20 Mar 2017 16:43, Joseph Myers wrote: > >> On Mon, 20 Mar 2017, Szabolcs Nagy wrote: > >>> in glibc it seems the rule was that there > >>> is space in function calls but not necessarily > >>> in macro invocation. > >> > >> Only certain macros that use concatenation to construct a variable name > >> (GLRO), type name (ElfW) etc. - where the result is conceptually a single > >> identifier. Macros used more like function calls should have the space. > >> That includes ones such as PSEUDO, END, weak_alias, hidden_def etc. > > > > do we want to formalize it ? > > https://sourceware.org/glibc/wiki/Style_and_Conventions#Symbols_and_Parenthesis > > > > macros that aren't "functions" should omit the space ? > > defined() counts as a function right ? > > Yes, we should formalize it. Please adjust the wiki. We can > always clarify further as time goes on or experience shows > otherwise. > > Yes, non-function macros should omit the space, and defined() > in my book counts as a function. done. i provided a number of examples to hopefully guide. -mike
On 03/22/2017 02:12 PM, Mike Frysinger wrote: > On 22 Mar 2017 12:16, Carlos O'Donell wrote: >> On 03/22/2017 02:05 AM, Mike Frysinger wrote: >>> On 20 Mar 2017 16:43, Joseph Myers wrote: >>>> On Mon, 20 Mar 2017, Szabolcs Nagy wrote: >>>>> in glibc it seems the rule was that there >>>>> is space in function calls but not necessarily >>>>> in macro invocation. >>>> >>>> Only certain macros that use concatenation to construct a variable name >>>> (GLRO), type name (ElfW) etc. - where the result is conceptually a single >>>> identifier. Macros used more like function calls should have the space. >>>> That includes ones such as PSEUDO, END, weak_alias, hidden_def etc. >>> >>> do we want to formalize it ? >>> https://sourceware.org/glibc/wiki/Style_and_Conventions#Symbols_and_Parenthesis >>> >>> macros that aren't "functions" should omit the space ? >>> defined() counts as a function right ? >> >> Yes, we should formalize it. Please adjust the wiki. We can >> always clarify further as time goes on or experience shows >> otherwise. >> >> Yes, non-function macros should omit the space, and defined() >> in my book counts as a function. > > done. i provided a number of examples to hopefully guide. > -mike Looks awesome. Thanks for those.
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index 24f75dbd9c78..6862bee34024 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -318,7 +318,12 @@ __spawnix (pid_t * pid, const char *file, /* Add a slack area for child's stack. */ size_t argv_size = (argc * sizeof (void *)) + 512; - size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize)); + /* We need at least a few pages in case the compiler's stack checking is + enabled. In some configs, it is known to use at least 24KiB. We use + 32KiB to be "safe" from anything the compiler might do. Besides, the + extra pages won't actually be allocated unless they get used. */ + argv_size += (32 * 1024); + size_t stack_size = ALIGN_UP (argv_size, GLRO (dl_pagesize)); void *stack = __mmap (NULL, stack_size, prot, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); if (__glibc_unlikely (stack == MAP_FAILED))