Message ID | 20171107223908.GB46190@intel.com |
---|---|
State | New |
Headers | show |
Series | [1/2] Add tst-jmp_buf.c and jmp_buf-macros.h | expand |
On 11/07/2017 11:39 PM, H.J. Lu wrote: > +typedef struct > + { > + __sigset_t __saved_mask; > + } __jmpbuf_target_t; “Target” in the sense of “target architecture”, not “jump target”? Maybe it's better to pick a different name. Should <bits/setjmp3.h> be a header under bits/types/ instead? Thanks, Florian
On Tue, Nov 7, 2017 at 10:05 PM, Florian Weimer <fweimer@redhat.com> wrote: > On 11/07/2017 11:39 PM, H.J. Lu wrote: >> >> +typedef struct >> + { >> + __sigset_t __saved_mask; >> + } __jmpbuf_target_t; > > > “Target” in the sense of “target architecture”, not “jump target”? Maybe > it's better to pick a different name. I checked it to __jmpbuf_arch_t. > Should <bits/setjmp3.h> be a header under bits/types/ instead? > I renamed it to bits/types/__jmpbuf_arch_t.h. Here is the updated patch. Tested with build-many-glibcs.py. Are there any comments or objections? Thanks.
On 11/08/2017 07:27 PM, H.J. Lu wrote: > +/* The biggest signal number + 1 */ > +#define _JUMP_BUF_SIGSET_NSIG 257 > +/* Number of longs to hold all signals. */ > +#define _JUMP_BUF_SIGSET_NWORDS \ > + ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) Where does 257 come from? 65 or 129 I would understand considering the kernel sources, but 257 is odd. I think it would be clearer to hard-code the array sizes and explain why the values where chosen in that way. We also need a test that setprocmask does not read from the previously unused part. I can move the existing next_to_fault bits to support/ if that would help. Thanks, Florian
On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 11/08/2017 07:27 PM, H.J. Lu wrote: >> >> +/* The biggest signal number + 1 */ >> +#define _JUMP_BUF_SIGSET_NSIG 257 >> +/* Number of longs to hold all signals. */ >> +#define _JUMP_BUF_SIGSET_NWORDS \ >> + ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) > > > Where does 257 come from? 65 or 129 I would understand considering the > kernel sources, but 257 is odd. I picked 257 to leave some room > I think it would be clearer to hard-code the array sizes and explain why the > values where chosen in that way. > > We also need a test that setprocmask does not read from the previously Did you mean "sigprocmask"? > unused part. I can move the existing next_to_fault bits to support/ if that > would help. Yes, please. Thanks.
On 11/13/2017 03:05 PM, H.J. Lu wrote: > On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 11/08/2017 07:27 PM, H.J. Lu wrote: >>> >>> +/* The biggest signal number + 1 */ >>> +#define _JUMP_BUF_SIGSET_NSIG 257 >>> +/* Number of longs to hold all signals. */ >>> +#define _JUMP_BUF_SIGSET_NWORDS \ >>> + ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) >> >> >> Where does 257 come from? 65 or 129 I would understand considering the >> kernel sources, but 257 is odd. Oh. I'm not sure if we should put this into installed header. Maybe we can use a different approach? Something similar to the pthread types? Or just not change the external type at all and just ad some internal space reuse mechanism? We had problems with people poking at supposedly invisible jmpbuf contents in the past, and I'm worried that adding even __ members will encourage that. >> I think it would be clearer to hard-code the array sizes and explain why the >> values where chosen in that way. >> >> We also need a test that setprocmask does not read from the previously > > Did you mean "sigprocmask"? Right. >> unused part. I can move the existing next_to_fault bits to support/ if that >> would help. > > Yes, please. Okay, I'll move it to support/ soon. Thanks, Florian
On Mon, Nov 13, 2017 at 8:34 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 11/13/2017 03:05 PM, H.J. Lu wrote: >> >> On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com> >> wrote: >>> >>> On 11/08/2017 07:27 PM, H.J. Lu wrote: >>>> >>>> >>>> +/* The biggest signal number + 1 */ >>>> +#define _JUMP_BUF_SIGSET_NSIG 257 >>>> +/* Number of longs to hold all signals. */ >>>> +#define _JUMP_BUF_SIGSET_NWORDS \ >>>> + ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) >>> >>> >>> >>> Where does 257 come from? 65 or 129 I would understand considering the >>> kernel sources, but 257 is odd. > > > Oh. I'm not sure if we should put this into installed header. > > Maybe we can use a different approach? Something similar to the pthread > types? Or just not change the external type at all and just ad some > internal space reuse mechanism? I will see what I can do. > We had problems with people poking at supposedly invisible jmpbuf contents > in the past, and I'm worried that adding even __ members will encourage > that. > >>> I think it would be clearer to hard-code the array sizes and explain why >>> the >>> values where chosen in that way. >>> >>> We also need a test that setprocmask does not read from the previously >> >> >> Did you mean "sigprocmask"? > > > Right. > >>> unused part. I can move the existing next_to_fault bits to support/ if >>> that >>> would help. >> >> >> Yes, please. > > > Okay, I'll move it to support/ soon. > Here is what the test will look like static int do_test (void) { struct __jmp_buf_tag *sj = memory from next_to_fault ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ "SJ" will point to the end of page in such a way that sigprocmask will fail if it reads beyond the actual __saved_mask. sigset_t m; sigemptyset (&m); sigprocmask (SIG_SETMASK, &m, NULL); if (sigsetjmp (sj, 0) == 0) { sigaddset (&m, SIGUSR1); sigprocmask (SIG_SETMASK, &m, NULL); siglongjmp (sj, 1); return EXIT_FAILURE; } sigprocmask (SIG_SETMASK, NULL, &m); return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE; } #define TEST_FUNCTION do_test () #include "../test-skeleton.c" Thanks.
On Mon, Nov 13, 2017 at 8:44 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Nov 13, 2017 at 8:34 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 11/13/2017 03:05 PM, H.J. Lu wrote: >>> >>> On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com> >>> wrote: >>>> >>>> On 11/08/2017 07:27 PM, H.J. Lu wrote: >>>>> >>>>> >>>>> +/* The biggest signal number + 1 */ >>>>> +#define _JUMP_BUF_SIGSET_NSIG 257 >>>>> +/* Number of longs to hold all signals. */ >>>>> +#define _JUMP_BUF_SIGSET_NWORDS \ >>>>> + ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) >>>> >>>> >>>> >>>> Where does 257 come from? 65 or 129 I would understand considering the >>>> kernel sources, but 257 is odd. >> >> >> Oh. I'm not sure if we should put this into installed header. >> >> Maybe we can use a different approach? Something similar to the pthread >> types? Or just not change the external type at all and just ad some >> internal space reuse mechanism? > > I will see what I can do. > >> We had problems with people poking at supposedly invisible jmpbuf contents >> in the past, and I'm worried that adding even __ members will encourage >> that. >> >>>> I think it would be clearer to hard-code the array sizes and explain why >>>> the >>>> values where chosen in that way. >>>> >>>> We also need a test that setprocmask does not read from the previously >>> >>> >>> Did you mean "sigprocmask"? >> >> >> Right. >> >>>> unused part. I can move the existing next_to_fault bits to support/ if >>>> that >>>> would help. >>> >>> >>> Yes, please. >> >> >> Okay, I'll move it to support/ soon. >> > Here is the updated patch to add <setjmpP.h>. I added a test: struct support_next_to_fault jmpbuf = support_next_to_fault_allocate (SAVED_MASK_OFFSET + (_NSIG / 8)); struct __jmp_buf_tag *sj = (struct __jmp_buf_tag *) jmpbuf.buffer; errno = 0; if (sigsetjmp (sj, 1) == 0) { siglongjmp (sj, 1); return EXIT_FAILURE; } if (errno != 0) { printf ("sigsetjmp: %s\n", strerror (errno)); return EXIT_FAILURE; } to verify that __sigprocmask won't read beyond _NSIG / 8. Does it look OK? Thanks.
On Mon, Nov 13, 2017 at 11:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Nov 13, 2017 at 8:44 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Mon, Nov 13, 2017 at 8:34 AM, Florian Weimer <fweimer@redhat.com> wrote: >>> On 11/13/2017 03:05 PM, H.J. Lu wrote: >>>> >>>> On Mon, Nov 13, 2017 at 5:09 AM, Florian Weimer <fweimer@redhat.com> >>>> wrote: >>>>> >>>>> On 11/08/2017 07:27 PM, H.J. Lu wrote: >>>>>> >>>>>> >>>>>> +/* The biggest signal number + 1 */ >>>>>> +#define _JUMP_BUF_SIGSET_NSIG 257 >>>>>> +/* Number of longs to hold all signals. */ >>>>>> +#define _JUMP_BUF_SIGSET_NWORDS \ >>>>>> + ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) >>>>> >>>>> >>>>> >>>>> Where does 257 come from? 65 or 129 I would understand considering the >>>>> kernel sources, but 257 is odd. >>> >>> >>> Oh. I'm not sure if we should put this into installed header. >>> >>> Maybe we can use a different approach? Something similar to the pthread >>> types? Or just not change the external type at all and just ad some >>> internal space reuse mechanism? >> >> I will see what I can do. >> >>> We had problems with people poking at supposedly invisible jmpbuf contents >>> in the past, and I'm worried that adding even __ members will encourage >>> that. >>> >>>>> I think it would be clearer to hard-code the array sizes and explain why >>>>> the >>>>> values where chosen in that way. >>>>> >>>>> We also need a test that setprocmask does not read from the previously >>>> >>>> >>>> Did you mean "sigprocmask"? >>> >>> >>> Right. >>> >>>>> unused part. I can move the existing next_to_fault bits to support/ if >>>>> that >>>>> would help. >>>> >>>> >>>> Yes, please. >>> >>> >>> Okay, I'll move it to support/ soon. >>> >> > > Here is the updated patch to add <setjmpP.h>. I added a test: > > struct support_next_to_fault jmpbuf > = support_next_to_fault_allocate (SAVED_MASK_OFFSET + (_NSIG / 8)); > struct __jmp_buf_tag *sj = (struct __jmp_buf_tag *) jmpbuf.buffer; > > errno = 0; > if (sigsetjmp (sj, 1) == 0) > { > siglongjmp (sj, 1); > return EXIT_FAILURE; > } > if (errno != 0) > { > printf ("sigsetjmp: %s\n", strerror (errno)); > return EXIT_FAILURE; > } > > to verify that __sigprocmask won't read beyond _NSIG / 8. > > Does it look OK? > This is the updated patch. Tested with build-many-glibcs.py. Any other comments? Thanks.
On 11/14/2017 12:22 AM, H.J. Lu wrote: > diff --git a/setjmp/tst-sigsetjmp2.c b/setjmp/tst-sigsetjmp2.c > new file mode 100644 > index 0000000000..131531feb0 > --- /dev/null > +++ b/setjmp/tst-sigsetjmp2.c > @@ -0,0 +1,50 @@ > +/* Test that sigprocmask does not read from the unused part of jmpbuf. > + Copyright (C) 2017 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > +<http://www.gnu.org/licenses/>. */ > + > +#include <stdlib.h> > +#include <signal.h> > +#include <setjmp.h> > +#include <stdio.h> > +#include <string.h> > +#include <errno.h> > +#include <jmp_buf-macros.h> > +#include <support/next_to_fault.h> > + > +static int > +do_test (void) > +{ > + struct support_next_to_fault jmpbuf > + = support_next_to_fault_allocate (SAVED_MASK_OFFSET + (_NSIG / 8)); > + struct __jmp_buf_tag *sj = (struct __jmp_buf_tag *) jmpbuf.buffer; > + > + errno = 0; > + if (sigsetjmp (sj, 1) == 0) > + { > + siglongjmp (sj, 1); > + return EXIT_FAILURE; > + } > + if (errno != 0) > + { > + printf ("sigsetjmp: %s\n", strerror (errno)); > + return EXIT_FAILURE; > + } > + return EXIT_SUCCESS; > +} > + > +#define TEST_FUNCTION do_test () > +#include "../test-skeleton.c" Please use <support/test-driver.c>. Wouldn't the test start to fail if sigsetjmp/siglongjmp actually started using the padding you are freeing up for use? I think you need test sigprocmask directly to ensure it has the desired property of not clobbering the padding. Thanks, Florian
On Tue, Nov 14, 2017 at 4:26 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 11/14/2017 12:22 AM, H.J. Lu wrote: >> >> diff --git a/setjmp/tst-sigsetjmp2.c b/setjmp/tst-sigsetjmp2.c >> new file mode 100644 >> index 0000000000..131531feb0 >> --- /dev/null >> +++ b/setjmp/tst-sigsetjmp2.c >> @@ -0,0 +1,50 @@ >> +/* Test that sigprocmask does not read from the unused part of jmpbuf. >> + Copyright (C) 2017 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> +<http://www.gnu.org/licenses/>. */ >> + >> +#include <stdlib.h> >> +#include <signal.h> >> +#include <setjmp.h> >> +#include <stdio.h> >> +#include <string.h> >> +#include <errno.h> >> +#include <jmp_buf-macros.h> >> +#include <support/next_to_fault.h> >> + >> +static int >> +do_test (void) >> +{ >> + struct support_next_to_fault jmpbuf >> + = support_next_to_fault_allocate (SAVED_MASK_OFFSET + (_NSIG / 8)); >> + struct __jmp_buf_tag *sj = (struct __jmp_buf_tag *) jmpbuf.buffer; >> + >> + errno = 0; >> + if (sigsetjmp (sj, 1) == 0) >> + { >> + siglongjmp (sj, 1); >> + return EXIT_FAILURE; >> + } >> + if (errno != 0) >> + { >> + printf ("sigsetjmp: %s\n", strerror (errno)); >> + return EXIT_FAILURE; >> + } >> + return EXIT_SUCCESS; >> +} >> + >> +#define TEST_FUNCTION do_test () >> +#include "../test-skeleton.c" > > > Please use <support/test-driver.c>. Done. > Wouldn't the test start to fail if sigsetjmp/siglongjmp actually started > using the padding you are freeing up for use? You are right. > I think you need test sigprocmask directly to ensure it has the desired > property of not clobbering the padding. Here is the updated patch. OK for master? Thanks.
On 11/14/2017 02:10 PM, H.J. Lu wrote: > +static int > +do_test (void) > +{ > + sigjmp_buf sj; > + struct support_next_to_fault sigset_t_buf > + = support_next_to_fault_allocate (SIZEOF_SIGSET_T); > + sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer; > + sigset_t m; > + > + sigemptyset (&m); > + memcpy (m_p, &m, SIZEOF_SIGSET_T); > + sigprocmask (SIG_SETMASK, m_p, NULL); > + memcpy (&m, m_p, SIZEOF_SIGSET_T); > + if (sigsetjmp (sj, 0) == 0) > + { > + sigaddset (&m, SIGUSR1); > + memcpy (m_p, &m, SIZEOF_SIGSET_T); > + sigprocmask (SIG_SETMASK, m_p, NULL); > + memcpy (&m, m_p, SIZEOF_SIGSET_T); > + siglongjmp (sj, 1); > + return EXIT_FAILURE; > + } > + sigprocmask (SIG_SETMASK, NULL, m_p); > + memcpy (&m, m_p, SIZEOF_SIGSET_T); > + return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE; > +} Sorry, I don't understand anymore what this test is supposed to test and how. To be honest, I don't like how you inject the internal definition of jmp_buf. Is this the way we do it for the nptl types? I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant (_NSIGS?) somewhere. Thanks, Florian
On Wed, Nov 15, 2017 at 3:10 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 11/14/2017 02:10 PM, H.J. Lu wrote: >> >> +static int >> +do_test (void) >> +{ >> + sigjmp_buf sj; >> + struct support_next_to_fault sigset_t_buf >> + = support_next_to_fault_allocate (SIZEOF_SIGSET_T); >> + sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer; >> + sigset_t m; >> + >> + sigemptyset (&m); >> + memcpy (m_p, &m, SIZEOF_SIGSET_T); >> + sigprocmask (SIG_SETMASK, m_p, NULL); >> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >> + if (sigsetjmp (sj, 0) == 0) >> + { >> + sigaddset (&m, SIGUSR1); >> + memcpy (m_p, &m, SIZEOF_SIGSET_T); >> + sigprocmask (SIG_SETMASK, m_p, NULL); >> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >> + siglongjmp (sj, 1); >> + return EXIT_FAILURE; >> + } >> + sigprocmask (SIG_SETMASK, NULL, m_p); >> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >> + return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE; >> +} > > > Sorry, I don't understand anymore what this test is supposed to test and > how. This tests the reduced __jmp_buf_sigset_t used by __saved_mask is bigger than sigset expected by kernel. > To be honest, I don't like how you inject the internal definition of > jmp_buf. Is this the way we do it for the nptl types? I only need to make some room in /* Calling environment, plus possibly a saved signal mask. */ struct __jmp_buf_tag { /* NOTE: The machine-dependent definitions of `__sigsetjmp' assume that a `jmp_buf' begins with a `__jmp_buf' and that `__mask_was_saved' follows it. Do not move these members or add others before it. */ __jmp_buf __jmpbuf; /* Calling environment. */ int __mask_was_saved; /* Saved the signal mask? */ __sigset_t __saved_mask; /* Saved signal mask. */ }; for target specific purpose. I changed it to struct __jmp_buf_tag { /* NOTE: The machine-dependent definitions of `__sigsetjmp' assume that a `jmp_buf' begins with a `__jmp_buf' and that `__mask_was_saved' follows it. Do not move these members or add others before it. */ __jmp_buf __jmpbuf; /* Calling environment. */ int __mask_was_saved; /* Saved the signal mask? */ union { __sigset_t __saved_mask_compat; struct { __jmp_buf_sigset_t __saved_mask; /* Paddings for architecture specific usage. */ unsigned long int __padding[12]; } __saved; } __saved_mask; }; #define __saved_mask __saved_mask.__saved.__saved_mask I did only to __sigset_t in __jmp_buf_tag and this test verifies that the size of __jmp_buf_sigset_t works with sigprocmask. Are you suggesting we make some room in __sigset_t directly? This will require very extensive changes. If not, what do you exactly suggest? > I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant > (_NSIGS?) somewhere. There is a _Static_assert in include/setjmp.h. But it isn't triggered. This updated patch moves it to sysdeps/unix/sysv/linux/__saved_mask.h. Now I got ../sysdeps/unix/sysv/linux/__saved_mask.h:32:1: error: static assertion failed: "size of ___saved_mask < size of __sigprocmask_sigset_t" _Static_assert (sizeof (___saved_mask) >= sizeof (__sigprocmask_sigset_t), if __jmp_buf_sigset_t is too small.
On Wed, Nov 15, 2017 at 5:20 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Wed, Nov 15, 2017 at 3:10 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 11/14/2017 02:10 PM, H.J. Lu wrote: >>> >>> +static int >>> +do_test (void) >>> +{ >>> + sigjmp_buf sj; >>> + struct support_next_to_fault sigset_t_buf >>> + = support_next_to_fault_allocate (SIZEOF_SIGSET_T); >>> + sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer; >>> + sigset_t m; >>> + >>> + sigemptyset (&m); >>> + memcpy (m_p, &m, SIZEOF_SIGSET_T); >>> + sigprocmask (SIG_SETMASK, m_p, NULL); >>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>> + if (sigsetjmp (sj, 0) == 0) >>> + { >>> + sigaddset (&m, SIGUSR1); >>> + memcpy (m_p, &m, SIZEOF_SIGSET_T); >>> + sigprocmask (SIG_SETMASK, m_p, NULL); >>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>> + siglongjmp (sj, 1); >>> + return EXIT_FAILURE; >>> + } >>> + sigprocmask (SIG_SETMASK, NULL, m_p); >>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>> + return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE; >>> +} >> >> >> Sorry, I don't understand anymore what this test is supposed to test and >> how. > > This tests the reduced __jmp_buf_sigset_t used by __saved_mask is > bigger than sigset expected by kernel. > >> To be honest, I don't like how you inject the internal definition of >> jmp_buf. Is this the way we do it for the nptl types? > > I only need to make some room in > > /* Calling environment, plus possibly a saved signal mask. */ > struct __jmp_buf_tag > { > /* NOTE: The machine-dependent definitions of `__sigsetjmp' > assume that a `jmp_buf' begins with a `__jmp_buf' and that > `__mask_was_saved' follows it. Do not move these members > or add others before it. */ > __jmp_buf __jmpbuf; /* Calling environment. */ > int __mask_was_saved; /* Saved the signal mask? */ > __sigset_t __saved_mask; /* Saved signal mask. */ > }; > > for target specific purpose. I changed it to > > struct __jmp_buf_tag > { > /* NOTE: The machine-dependent definitions of `__sigsetjmp' > assume that a `jmp_buf' begins with a `__jmp_buf' and that > `__mask_was_saved' follows it. Do not move these members > or add others before it. */ > __jmp_buf __jmpbuf; /* Calling environment. */ > int __mask_was_saved; /* Saved the signal mask? */ > union > { > __sigset_t __saved_mask_compat; > struct > { > __jmp_buf_sigset_t __saved_mask; > /* Paddings for architecture specific usage. */ > unsigned long int __padding[12]; > } __saved; > } __saved_mask; > }; > > #define __saved_mask __saved_mask.__saved.__saved_mask > > I did only to __sigset_t in __jmp_buf_tag and this test verifies that the size > of __jmp_buf_sigset_t works with sigprocmask. > > Are you suggesting we make some room in __sigset_t directly? This will > require very extensive changes. If not, what do you exactly suggest? > >> I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant >> (_NSIGS?) somewhere. > > There is a _Static_assert in include/setjmp.h. But it isn't triggered. This > updated patch moves it to sysdeps/unix/sysv/linux/__saved_mask.h. Now > I got > > ../sysdeps/unix/sysv/linux/__saved_mask.h:32:1: error: static > assertion failed: "size of ___saved_mask < size of > __sigprocmask_sigset_t" > _Static_assert (sizeof (___saved_mask) >= sizeof (__sigprocmask_sigset_t), > > if __jmp_buf_sigset_t is too small. > > -- > H.J. Any comments, suggestions or objections? The patch is at https://sourceware.org/ml/libc-alpha/2017-11/msg00510.html
On Tue, Nov 21, 2017 at 2:22 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Wed, Nov 15, 2017 at 5:20 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Wed, Nov 15, 2017 at 3:10 AM, Florian Weimer <fweimer@redhat.com> wrote: >>> On 11/14/2017 02:10 PM, H.J. Lu wrote: >>>> >>>> +static int >>>> +do_test (void) >>>> +{ >>>> + sigjmp_buf sj; >>>> + struct support_next_to_fault sigset_t_buf >>>> + = support_next_to_fault_allocate (SIZEOF_SIGSET_T); >>>> + sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer; >>>> + sigset_t m; >>>> + >>>> + sigemptyset (&m); >>>> + memcpy (m_p, &m, SIZEOF_SIGSET_T); >>>> + sigprocmask (SIG_SETMASK, m_p, NULL); >>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>>> + if (sigsetjmp (sj, 0) == 0) >>>> + { >>>> + sigaddset (&m, SIGUSR1); >>>> + memcpy (m_p, &m, SIZEOF_SIGSET_T); >>>> + sigprocmask (SIG_SETMASK, m_p, NULL); >>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>>> + siglongjmp (sj, 1); >>>> + return EXIT_FAILURE; >>>> + } >>>> + sigprocmask (SIG_SETMASK, NULL, m_p); >>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>>> + return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE; >>>> +} >>> >>> >>> Sorry, I don't understand anymore what this test is supposed to test and >>> how. >> >> This tests the reduced __jmp_buf_sigset_t used by __saved_mask is >> bigger than sigset expected by kernel. >> >>> To be honest, I don't like how you inject the internal definition of >>> jmp_buf. Is this the way we do it for the nptl types? >> >> I only need to make some room in >> >> /* Calling environment, plus possibly a saved signal mask. */ >> struct __jmp_buf_tag >> { >> /* NOTE: The machine-dependent definitions of `__sigsetjmp' >> assume that a `jmp_buf' begins with a `__jmp_buf' and that >> `__mask_was_saved' follows it. Do not move these members >> or add others before it. */ >> __jmp_buf __jmpbuf; /* Calling environment. */ >> int __mask_was_saved; /* Saved the signal mask? */ >> __sigset_t __saved_mask; /* Saved signal mask. */ >> }; >> >> for target specific purpose. I changed it to >> >> struct __jmp_buf_tag >> { >> /* NOTE: The machine-dependent definitions of `__sigsetjmp' >> assume that a `jmp_buf' begins with a `__jmp_buf' and that >> `__mask_was_saved' follows it. Do not move these members >> or add others before it. */ >> __jmp_buf __jmpbuf; /* Calling environment. */ >> int __mask_was_saved; /* Saved the signal mask? */ >> union >> { >> __sigset_t __saved_mask_compat; >> struct >> { >> __jmp_buf_sigset_t __saved_mask; >> /* Paddings for architecture specific usage. */ >> unsigned long int __padding[12]; >> } __saved; >> } __saved_mask; >> }; >> >> #define __saved_mask __saved_mask.__saved.__saved_mask >> >> I did only to __sigset_t in __jmp_buf_tag and this test verifies that the size >> of __jmp_buf_sigset_t works with sigprocmask. >> >> Are you suggesting we make some room in __sigset_t directly? This will >> require very extensive changes. If not, what do you exactly suggest? >> >>> I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant >>> (_NSIGS?) somewhere. >> >> There is a _Static_assert in include/setjmp.h. But it isn't triggered. This >> updated patch moves it to sysdeps/unix/sysv/linux/__saved_mask.h. Now >> I got >> >> ../sysdeps/unix/sysv/linux/__saved_mask.h:32:1: error: static >> assertion failed: "size of ___saved_mask < size of >> __sigprocmask_sigset_t" >> _Static_assert (sizeof (___saved_mask) >= sizeof (__sigprocmask_sigset_t), >> >> if __jmp_buf_sigset_t is too small. >> >> -- >> H.J. > > Any comments, suggestions or objections? The patch is at > > https://sourceware.org/ml/libc-alpha/2017-11/msg00510.html > This is the updated patch. It limited the changes to Linux/x86. Any comments?
On Sat, Nov 25, 2017 at 8:34 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Tue, Nov 21, 2017 at 2:22 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Wed, Nov 15, 2017 at 5:20 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Wed, Nov 15, 2017 at 3:10 AM, Florian Weimer <fweimer@redhat.com> wrote: >>>> On 11/14/2017 02:10 PM, H.J. Lu wrote: >>>>> >>>>> +static int >>>>> +do_test (void) >>>>> +{ >>>>> + sigjmp_buf sj; >>>>> + struct support_next_to_fault sigset_t_buf >>>>> + = support_next_to_fault_allocate (SIZEOF_SIGSET_T); >>>>> + sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer; >>>>> + sigset_t m; >>>>> + >>>>> + sigemptyset (&m); >>>>> + memcpy (m_p, &m, SIZEOF_SIGSET_T); >>>>> + sigprocmask (SIG_SETMASK, m_p, NULL); >>>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>>>> + if (sigsetjmp (sj, 0) == 0) >>>>> + { >>>>> + sigaddset (&m, SIGUSR1); >>>>> + memcpy (m_p, &m, SIZEOF_SIGSET_T); >>>>> + sigprocmask (SIG_SETMASK, m_p, NULL); >>>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>>>> + siglongjmp (sj, 1); >>>>> + return EXIT_FAILURE; >>>>> + } >>>>> + sigprocmask (SIG_SETMASK, NULL, m_p); >>>>> + memcpy (&m, m_p, SIZEOF_SIGSET_T); >>>>> + return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE; >>>>> +} >>>> >>>> >>>> Sorry, I don't understand anymore what this test is supposed to test and >>>> how. >>> >>> This tests the reduced __jmp_buf_sigset_t used by __saved_mask is >>> bigger than sigset expected by kernel. >>> >>>> To be honest, I don't like how you inject the internal definition of >>>> jmp_buf. Is this the way we do it for the nptl types? >>> >>> I only need to make some room in >>> >>> /* Calling environment, plus possibly a saved signal mask. */ >>> struct __jmp_buf_tag >>> { >>> /* NOTE: The machine-dependent definitions of `__sigsetjmp' >>> assume that a `jmp_buf' begins with a `__jmp_buf' and that >>> `__mask_was_saved' follows it. Do not move these members >>> or add others before it. */ >>> __jmp_buf __jmpbuf; /* Calling environment. */ >>> int __mask_was_saved; /* Saved the signal mask? */ >>> __sigset_t __saved_mask; /* Saved signal mask. */ >>> }; >>> >>> for target specific purpose. I changed it to >>> >>> struct __jmp_buf_tag >>> { >>> /* NOTE: The machine-dependent definitions of `__sigsetjmp' >>> assume that a `jmp_buf' begins with a `__jmp_buf' and that >>> `__mask_was_saved' follows it. Do not move these members >>> or add others before it. */ >>> __jmp_buf __jmpbuf; /* Calling environment. */ >>> int __mask_was_saved; /* Saved the signal mask? */ >>> union >>> { >>> __sigset_t __saved_mask_compat; >>> struct >>> { >>> __jmp_buf_sigset_t __saved_mask; >>> /* Paddings for architecture specific usage. */ >>> unsigned long int __padding[12]; >>> } __saved; >>> } __saved_mask; >>> }; >>> >>> #define __saved_mask __saved_mask.__saved.__saved_mask >>> >>> I did only to __sigset_t in __jmp_buf_tag and this test verifies that the size >>> of __jmp_buf_sigset_t works with sigprocmask. >>> >>> Are you suggesting we make some room in __sigset_t directly? This will >>> require very extensive changes. If not, what do you exactly suggest? >>> >>>> I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant >>>> (_NSIGS?) somewhere. >>> >>> There is a _Static_assert in include/setjmp.h. But it isn't triggered. This >>> updated patch moves it to sysdeps/unix/sysv/linux/__saved_mask.h. Now >>> I got >>> >>> ../sysdeps/unix/sysv/linux/__saved_mask.h:32:1: error: static >>> assertion failed: "size of ___saved_mask < size of >>> __sigprocmask_sigset_t" >>> _Static_assert (sizeof (___saved_mask) >= sizeof (__sigprocmask_sigset_t), >>> >>> if __jmp_buf_sigset_t is too small. >>> >>> -- >>> H.J. >> >> Any comments, suggestions or objections? The patch is at >> >> https://sourceware.org/ml/libc-alpha/2017-11/msg00510.html >> > > This is the updated patch. It limited the changes to Linux/x86. > Any comments? > This is the patch I am checking in tomorrow. Andrew, you can include <jmp_buf-ssp.h> to get SHADOW_STACK_POINTER_OFFSET in jmp_buf.
diff --git a/bits/setjmp3.h b/bits/setjmp3.h new file mode 100644 index 0000000000..b164c8d449 --- /dev/null +++ b/bits/setjmp3.h @@ -0,0 +1,31 @@ +/* Generic __jmpbuf_target_t defition. + Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#ifndef _SETJMP_H +# error "Never include <bits/setjmp3.h> directly; use <setjmp.h> instead." +#endif + +#include <bits/types/__sigset_t.h> + +typedef struct + { + __sigset_t __saved_mask; + } __jmpbuf_target_t; + +/* Saved signal mask. */ +#define __saved_mask __target.__saved_mask diff --git a/setjmp/Makefile b/setjmp/Makefile index 857e0b8d0f..6ab9b4c3ca 100644 --- a/setjmp/Makefile +++ b/setjmp/Makefile @@ -22,7 +22,7 @@ subdir := setjmp include ../Makeconfig -headers := setjmp.h bits/setjmp.h bits/setjmp2.h +headers := setjmp.h bits/setjmp.h bits/setjmp2.h bits/setjmp3.h routines := setjmp sigjmp bsd-setjmp bsd-_setjmp \ longjmp __longjmp jmp-unwind diff --git a/setjmp/longjmp.c b/setjmp/longjmp.c index 2453c2c124..86a024b4b8 100644 --- a/setjmp/longjmp.c +++ b/setjmp/longjmp.c @@ -31,7 +31,8 @@ __libc_siglongjmp (sigjmp_buf env, int val) if (env[0].__mask_was_saved) /* Restore the saved signal mask. */ - (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask, + (void) __sigprocmask (SIG_SETMASK, + (sigset_t *) &env[0].__saved_mask, (sigset_t *) NULL); /* Call the machine-dependent function to restore machine state. */ diff --git a/setjmp/setjmp.h b/setjmp/setjmp.h index 86fb2edf6c..1082fb0142 100644 --- a/setjmp/setjmp.h +++ b/setjmp/setjmp.h @@ -27,7 +27,7 @@ __BEGIN_DECLS #include <bits/setjmp.h> /* Get `__jmp_buf'. */ -#include <bits/types/__sigset_t.h> +#include <bits/setjmp3.h> /* Calling environment, plus possibly a saved signal mask. */ struct __jmp_buf_tag @@ -38,7 +38,7 @@ struct __jmp_buf_tag or add others before it. */ __jmp_buf __jmpbuf; /* Calling environment. */ int __mask_was_saved; /* Saved the signal mask? */ - __sigset_t __saved_mask; /* Saved signal mask. */ + __jmpbuf_target_t __target; /* Target specific data. */ }; diff --git a/setjmp/sigjmp.c b/setjmp/sigjmp.c index 30839ae819..0cd733b3ee 100644 --- a/setjmp/sigjmp.c +++ b/setjmp/sigjmp.c @@ -28,7 +28,7 @@ __sigjmp_save (sigjmp_buf env, int savemask) { env[0].__mask_was_saved = (savemask && __sigprocmask (SIG_BLOCK, (sigset_t *) NULL, - &env[0].__saved_mask) == 0); + (sigset_t *) &env[0].__saved_mask) == 0); return 0; } diff --git a/setjmp/tst-jmp_buf.c b/setjmp/tst-jmp_buf.c index 1b25d85876..16ddebc224 100644 --- a/setjmp/tst-jmp_buf.c +++ b/setjmp/tst-jmp_buf.c @@ -18,9 +18,19 @@ #include <stdio.h> #include <setjmp.h> +#include <signal.h> #include <stddef.h> #include <jmp_buf-macros.h> +#ifdef _NSIG +# define KERNEL_NSIG_WORDS (_NSIG / (8 * sizeof (unsigned long int))) + +typedef struct + { + unsigned long int __val[KERNEL_NSIG_WORDS]; + } kernel_sigset_t; +#endif + #define STR_HELPER(x) #x #define STR(x) STR_HELPER(x) @@ -54,6 +64,13 @@ do_test (void) TEST_OFFSET (struct __jmp_buf_tag, __saved_mask, SAVED_MASK_OFFSET); +#ifdef _NSIG + jmp_buf buf; + __typeof (buf[0].__saved_mask) saved_mask; + _Static_assert (sizeof (saved_mask) >= sizeof (kernel_sigset_t), + "size of saved_mask < size of kernel_sigset_t"); +#endif + return 0; } diff --git a/sysdeps/powerpc/longjmp.c b/sysdeps/powerpc/longjmp.c index bd3ed8c22b..a23f2c582b 100644 --- a/sysdeps/powerpc/longjmp.c +++ b/sysdeps/powerpc/longjmp.c @@ -39,7 +39,8 @@ __vmx__libc_siglongjmp (sigjmp_buf env, int val) if (env[0].__mask_was_saved) /* Restore the saved signal mask. */ - (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask, + (void) __sigprocmask (SIG_SETMASK, + (sigset_t *) &env[0].__saved_mask, (sigset_t *) NULL); /* Call the machine-dependent function to restore machine state. */ diff --git a/sysdeps/powerpc/novmx-longjmp.c b/sysdeps/powerpc/novmx-longjmp.c index b0020b728a..662fbc5432 100644 --- a/sysdeps/powerpc/novmx-longjmp.c +++ b/sysdeps/powerpc/novmx-longjmp.c @@ -37,7 +37,8 @@ __novmx__libc_siglongjmp (__novmx__sigjmp_buf env, int val) if (env[0].__mask_was_saved) /* Restore the saved signal mask. */ - (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask, + (void) __sigprocmask (SIG_SETMASK, + (sigset_t *) &env[0].__saved_mask, (sigset_t *) NULL); /* Call the machine-dependent function to restore machine state. */ diff --git a/sysdeps/powerpc/novmx-sigjmp.c b/sysdeps/powerpc/novmx-sigjmp.c index 7d0ae59437..62680fe38f 100644 --- a/sysdeps/powerpc/novmx-sigjmp.c +++ b/sysdeps/powerpc/novmx-sigjmp.c @@ -35,7 +35,7 @@ __novmx__sigjmp_save (__novmx__sigjmp_buf env, int savemask) { env[0].__mask_was_saved = (savemask && __sigprocmask (SIG_BLOCK, (sigset_t *) NULL, - &env[0].__saved_mask) == 0); + (sigset_t *) &env[0].__saved_mask) == 0); return 0; } diff --git a/sysdeps/powerpc/sigjmp.c b/sysdeps/powerpc/sigjmp.c index 6d593a0992..736491eff9 100644 --- a/sysdeps/powerpc/sigjmp.c +++ b/sysdeps/powerpc/sigjmp.c @@ -31,7 +31,7 @@ __vmx__sigjmp_save (sigjmp_buf env, int savemask) { env[0].__mask_was_saved = (savemask && __sigprocmask (SIG_BLOCK, (sigset_t *) NULL, - &env[0].__saved_mask) == 0); + (sigset_t *) &env[0].__saved_mask) == 0); return 0; } diff --git a/sysdeps/unix/sysv/linux/bits/setjmp3.h b/sysdeps/unix/sysv/linux/bits/setjmp3.h new file mode 100644 index 0000000000..e86dbdcba4 --- /dev/null +++ b/sysdeps/unix/sysv/linux/bits/setjmp3.h @@ -0,0 +1,48 @@ +/* __jmpbuf_target_t defition for Linux. + Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#ifndef _SETJMP_H +# error "Never include <bits/setjmp3.h> directly; use <setjmp.h> instead." +#endif + +#include <bits/types/__sigset_t.h> + +/* The biggest signal number + 1 */ +#define _JUMP_BUF_SIGSET_NSIG 257 +/* Number of longs to hold all signals. */ +#define _JUMP_BUF_SIGSET_NWORDS \ + ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) + +typedef struct + { + unsigned long int __val[_JUMP_BUF_SIGSET_NWORDS]; + } __jmp_buf_sigset_t; + +typedef union + { + __sigset_t __saved_mask_compat; + struct + { + __jmp_buf_sigset_t __saved_mask; + /* Paddings for target specific usage. */ + unsigned long int __padding[12]; + } __saved; + } __jmpbuf_target_t; + +/* Saved signal mask. */ +#define __saved_mask __target.__saved.__saved_mask diff --git a/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c b/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c index 874ed18d6b..658a25b87b 100644 --- a/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c +++ b/sysdeps/unix/sysv/linux/ia64/unwind_longjmp.c @@ -34,7 +34,8 @@ __libc_unwind_longjmp (sigjmp_buf env, int val) if (env[0].__mask_was_saved) /* Restore the saved signal mask. */ - (void) __sigprocmask (SIG_SETMASK, &env[0].__saved_mask, + (void) __sigprocmask (SIG_SETMASK, + (sigset_t *) &env[0].__saved_mask, (sigset_t *) NULL); /* Call the machine-dependent function to restore machine state. */