| Message ID | 20191210183221.26912-5-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/7] Fix __libc_signal_block_all on sparc64 | expand |
* Adhemerval Zanella: > The sigfillset already does it, and this is the canonical way to operate > on sigset_t. The only way to actually broke this assumption is if caller > initialize sigset with memset or something similar, i.e, bypassing glibc > (and again this is not a valid construction). Is you argument that sigfillset already does this, and there is no way to compute the complement of a signal set, so the bits for SIGCANCEL and SIGSETXID can never become set? I think it's possible to set them directly using sigaddset. I don't see why using SIGCANCEL/SIGSETXID with that function would be undefined. Thanks, Florian
On 12/12/2019 09:54, Florian Weimer wrote: > * Adhemerval Zanella: > >> The sigfillset already does it, and this is the canonical way to operate >> on sigset_t. The only way to actually broke this assumption is if caller >> initialize sigset with memset or something similar, i.e, bypassing glibc >> (and again this is not a valid construction). > > Is you argument that sigfillset already does this, and there is no way > to compute the complement of a signal set, so the bits for > SIGCANCEL and SIGSETXID can never become set? Yes. > > I think it's possible to set them directly using sigaddset. I don't see > why using SIGCANCEL/SIGSETXID with that function would be undefined. sigaddset filter out SIGCANCEL/SIGSETXID through __is_internal_signal, returning EINVAL for such case.
* Adhemerval Zanella: > On 12/12/2019 09:54, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> The sigfillset already does it, and this is the canonical way to operate >>> on sigset_t. The only way to actually broke this assumption is if caller >>> initialize sigset with memset or something similar, i.e, bypassing glibc >>> (and again this is not a valid construction). >> >> Is you argument that sigfillset already does this, and there is no way >> to compute the complement of a signal set, so the bits for >> SIGCANCEL and SIGSETXID can never become set? > > Yes. > >> >> I think it's possible to set them directly using sigaddset. I don't see >> why using SIGCANCEL/SIGSETXID with that function would be undefined. > > sigaddset filter out SIGCANCEL/SIGSETXID through __is_internal_signal, > returning EINVAL for such case. Oh, I see. It's still not clear to me whether it is not in fact better to allow appplications to block internal signals (from a compatibility perspective, e.g. if the application knows that the stack pointer is problematic). Thanks, Florian
On 12/12/2019 10:12, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 12/12/2019 09:54, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> The sigfillset already does it, and this is the canonical way to operate >>>> on sigset_t. The only way to actually broke this assumption is if caller >>>> initialize sigset with memset or something similar, i.e, bypassing glibc >>>> (and again this is not a valid construction). >>> >>> Is you argument that sigfillset already does this, and there is no way >>> to compute the complement of a signal set, so the bits for >>> SIGCANCEL and SIGSETXID can never become set? >> >> Yes. >> >>> >>> I think it's possible to set them directly using sigaddset. I don't see >>> why using SIGCANCEL/SIGSETXID with that function would be undefined. >> >> sigaddset filter out SIGCANCEL/SIGSETXID through __is_internal_signal, >> returning EINVAL for such case. > > Oh, I see. > > It's still not clear to me whether it is not in fact better to allow > appplications to block internal signals (from a compatibility > perspective, e.g. if the application knows that the stack pointer is > problematic). My view is the semantic of the signals are not exported to userspace (we could use a different signal for SIGCANCEL in a future version, for instance) and we can eventually phase out the signal usage if either POSIX deprecate some functionality or if kernel provides a cleanly way to accomplish the required functionality (for instance, if it provides a syscall that change the xid of all threads). Application can still block internal signals, but they will to actually statically initialize a sigprocmask in a non standard way.
On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 12/12/2019 10:12, Florian Weimer wrote: > > * Adhemerval Zanella: > > > >> On 12/12/2019 09:54, Florian Weimer wrote: > >>> * Adhemerval Zanella: > >>> > >>>> The sigfillset already does it, and this is the canonical way to operate > >>>> on sigset_t. The only way to actually broke this assumption is if caller > >>>> initialize sigset with memset or something similar, i.e, bypassing glibc > >>>> (and again this is not a valid construction). I think it would be appropriate for us to guarantee that `memset(s, 0, sizeof(sigset_t))` has the same effect as `sigemptyset(s)`, because I bet there is real code that does that, probably without realizing it's technically wrong (e.g. by using memset to wipe an entire struct sigaction and then not bothering to do a separate sigemptyset on sa_mask, or by statically allocating a sigset_t and assuming that zero-initialization will produce the same effect as sigemptyset). But that argument doesn't apply to `memset(s, 0xFF, sizeof(sigset_t))`. > > It's still not clear to me whether it is not in fact better to allow > > appplications to block internal signals (from a compatibility > > perspective, e.g. if the application knows that the stack pointer is > > problematic). > > My view is the semantic of the signals are not exported to userspace > (we could use a different signal for SIGCANCEL in a future version, > for instance) and we can eventually phase out the signal usage if > either POSIX deprecate some functionality or if kernel provides a > cleanly way to accomplish the required functionality (for instance, > if it provides a syscall that change the xid of all threads). > > Application can still block internal signals, but they will to actually > statically initialize a sigprocmask in a non standard way. This seems like a larger discussion that shouldn't hold up this patch. Status quo is that blocking SIGCANCEL and SIGSETXID is not supported, and the patch doesn't change that. zw
On 12/12/2019 14:59, Zack Weinberg wrote: > On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: >> On 12/12/2019 10:12, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> On 12/12/2019 09:54, Florian Weimer wrote: >>>>> * Adhemerval Zanella: >>>>> >>>>>> The sigfillset already does it, and this is the canonical way to operate >>>>>> on sigset_t. The only way to actually broke this assumption is if caller >>>>>> initialize sigset with memset or something similar, i.e, bypassing glibc >>>>>> (and again this is not a valid construction). > > I think it would be appropriate for us to guarantee that `memset(s, 0, > sizeof(sigset_t))` has the same effect as `sigemptyset(s)`, because I > bet there is real code that does that, probably without realizing it's > technically wrong (e.g. by using memset to wipe an entire struct > sigaction and then not bothering to do a separate sigemptyset on > sa_mask, or by statically allocating a sigset_t and assuming that > zero-initialization will produce the same effect as sigemptyset). > > But that argument doesn't apply to `memset(s, 0xFF, sizeof(sigset_t))`. In this case we will to either keep the current semantic or remove any filter in sigprocmark, sigemptyset, sigfillset, sigaddset, and sigdelset. > >>> It's still not clear to me whether it is not in fact better to allow >>> appplications to block internal signals (from a compatibility >>> perspective, e.g. if the application knows that the stack pointer is >>> problematic). >> >> My view is the semantic of the signals are not exported to userspace >> (we could use a different signal for SIGCANCEL in a future version, >> for instance) and we can eventually phase out the signal usage if >> either POSIX deprecate some functionality or if kernel provides a >> cleanly way to accomplish the required functionality (for instance, >> if it provides a syscall that change the xid of all threads). >> >> Application can still block internal signals, but they will to actually >> statically initialize a sigprocmask in a non standard way. > > This seems like a larger discussion that shouldn't hold up this patch. > Status quo is that blocking SIGCANCEL and SIGSETXID is not supported, > and the patch doesn't change that. In fact, the static / memset initialization is a point that made me realize that there is no much gain in this change. I think it is better to withdrew this patch.
On 12/12/2019 16:05, Adhemerval Zanella wrote: > > > On 12/12/2019 14:59, Zack Weinberg wrote: >> On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella >> <adhemerval.zanella@linaro.org> wrote: >>> On 12/12/2019 10:12, Florian Weimer wrote: >>>> * Adhemerval Zanella: >>>> >>>>> On 12/12/2019 09:54, Florian Weimer wrote: >>>>>> * Adhemerval Zanella: >>>>>> >>>>>>> The sigfillset already does it, and this is the canonical way to operate >>>>>>> on sigset_t. The only way to actually broke this assumption is if caller >>>>>>> initialize sigset with memset or something similar, i.e, bypassing glibc >>>>>>> (and again this is not a valid construction). >> >> I think it would be appropriate for us to guarantee that `memset(s, 0, >> sizeof(sigset_t))` has the same effect as `sigemptyset(s)`, because I >> bet there is real code that does that, probably without realizing it's >> technically wrong (e.g. by using memset to wipe an entire struct >> sigaction and then not bothering to do a separate sigemptyset on >> sa_mask, or by statically allocating a sigset_t and assuming that >> zero-initialization will produce the same effect as sigemptyset). >> >> But that argument doesn't apply to `memset(s, 0xFF, sizeof(sigset_t))`. > > In this case we will to either keep the current semantic or remove any > filter in sigprocmark, sigemptyset, sigfillset, sigaddset, and sigdelset. > >> >>>> It's still not clear to me whether it is not in fact better to allow >>>> appplications to block internal signals (from a compatibility >>>> perspective, e.g. if the application knows that the stack pointer is >>>> problematic). >>> >>> My view is the semantic of the signals are not exported to userspace >>> (we could use a different signal for SIGCANCEL in a future version, >>> for instance) and we can eventually phase out the signal usage if >>> either POSIX deprecate some functionality or if kernel provides a >>> cleanly way to accomplish the required functionality (for instance, >>> if it provides a syscall that change the xid of all threads). >>> >>> Application can still block internal signals, but they will to actually >>> statically initialize a sigprocmask in a non standard way. >> >> This seems like a larger discussion that shouldn't hold up this patch. >> Status quo is that blocking SIGCANCEL and SIGSETXID is not supported, >> and the patch doesn't change that. > > In fact, the static / memset initialization is a point that made me > realize that there is no much gain in this change. I think it is > better to withdrew this patch. > Maybe an option should be to explicit return EINVAL if user tries to set any internal signal instead of silent removed it and issue the sigprocmask?
On Thu, Dec 12, 2019 at 2:05 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 12/12/2019 14:59, Zack Weinberg wrote: > > On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella > > <adhemerval.zanella@linaro.org> wrote: > >> On 12/12/2019 10:12, Florian Weimer wrote: > >>> * Adhemerval Zanella: > >>> > >>>> On 12/12/2019 09:54, Florian Weimer wrote: > >>>>> * Adhemerval Zanella: > >>>>> > >>>>>> The sigfillset already does it, and this is the canonical way to operate > >>>>>> on sigset_t. The only way to actually broke this assumption is if caller > >>>>>> initialize sigset with memset or something similar, i.e, bypassing glibc > >>>>>> (and again this is not a valid construction). > > > > I think it would be appropriate for us to guarantee that `memset(s, 0, > > sizeof(sigset_t))` has the same effect as `sigemptyset(s)`, because I > > bet there is real code that does that, probably without realizing it's > > technically wrong (e.g. by using memset to wipe an entire struct > > sigaction and then not bothering to do a separate sigemptyset on > > sa_mask, or by statically allocating a sigset_t and assuming that > > zero-initialization will produce the same effect as sigemptyset). > > > > But that argument doesn't apply to `memset(s, 0xFF, sizeof(sigset_t))`. > > In this case we will to either keep the current semantic or remove any > filter in sigprocmark, sigemptyset, sigfillset, sigaddset, and sigdelset. I think I didn't explain myself clearly enough; I am _not_ pointing out a problem with your proposed patch. We don't support blocking SIGSETXID and SIGCANCEL, but we don't care if someone tries to unblock them, because they never get blocked in the first place. So `memset(s, 0, sizeof(sigset_t))` currently _does_ have the same effect as `sigemptyset(s)` and your patch wouldn't change that. And `memset(s, 0xFF, sizeof(sigset_t))` currently _doesn't_ have the same effect as `sigfillset(s)` and, again, your patch wouldn't change that. And I think we need to support `memset(s, 0, sizeof(sigset_t))` but not `memset(s, 0xFF, sizeof(sigset_t))`. zw
On 12/12/2019 16:29, Zack Weinberg wrote: > On Thu, Dec 12, 2019 at 2:05 PM Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: >> On 12/12/2019 14:59, Zack Weinberg wrote: >>> On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella >>> <adhemerval.zanella@linaro.org> wrote: >>>> On 12/12/2019 10:12, Florian Weimer wrote: >>>>> * Adhemerval Zanella: >>>>> >>>>>> On 12/12/2019 09:54, Florian Weimer wrote: >>>>>>> * Adhemerval Zanella: >>>>>>> >>>>>>>> The sigfillset already does it, and this is the canonical way to operate >>>>>>>> on sigset_t. The only way to actually broke this assumption is if caller >>>>>>>> initialize sigset with memset or something similar, i.e, bypassing glibc >>>>>>>> (and again this is not a valid construction). >>> >>> I think it would be appropriate for us to guarantee that `memset(s, 0, >>> sizeof(sigset_t))` has the same effect as `sigemptyset(s)`, because I >>> bet there is real code that does that, probably without realizing it's >>> technically wrong (e.g. by using memset to wipe an entire struct >>> sigaction and then not bothering to do a separate sigemptyset on >>> sa_mask, or by statically allocating a sigset_t and assuming that >>> zero-initialization will produce the same effect as sigemptyset). >>> >>> But that argument doesn't apply to `memset(s, 0xFF, sizeof(sigset_t))`. >> >> In this case we will to either keep the current semantic or remove any >> filter in sigprocmark, sigemptyset, sigfillset, sigaddset, and sigdelset. > > I think I didn't explain myself clearly enough; I am _not_ pointing > out a problem with your proposed patch. We don't support blocking > SIGSETXID and SIGCANCEL, but we don't care if someone tries to unblock > them, because they never get blocked in the first place. So > `memset(s, 0, sizeof(sigset_t))` currently _does_ have the same effect > as `sigemptyset(s)` and your patch wouldn't change that. And > `memset(s, 0xFF, sizeof(sigset_t))` currently _doesn't_ have the same > effect as `sigfillset(s)` and, again, your patch wouldn't change that. > And I think we need to support `memset(s, 0, sizeof(sigset_t))` but > not `memset(s, 0xFF, sizeof(sigset_t))`. Yeah I got you what you meant and my point of withdrew this patch is sigprocmask should indeed filter against unwarranted changes in sigset_t (by tricks like casting to a different type or memset 0xff). And that's why maybe a better option is to fail and alert the user that trying to block reserved signals are not allowed (by returning EINVAL).
diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c index 73b0d0c19a..c6961a8ac4 100644 --- a/sysdeps/unix/sysv/linux/sigprocmask.c +++ b/sysdeps/unix/sysv/linux/sigprocmask.c @@ -16,26 +16,12 @@ <https://www.gnu.org/licenses/>. */ #include <signal.h> -#include <nptl/pthreadP.h> /* SIGCANCEL, SIGSETXID */ +#include <sysdep.h> /* Get and/or change the set of blocked signals. */ int __sigprocmask (int how, const sigset_t *set, sigset_t *oset) { - sigset_t local_newmask; - - /* The only thing we have to make sure here is that SIGCANCEL and - SIGSETXID are not blocked. */ - if (set != NULL - && __glibc_unlikely (__sigismember (set, SIGCANCEL) - || __glibc_unlikely (__sigismember (set, SIGSETXID)))) - { - local_newmask = *set; - __sigdelset (&local_newmask, SIGCANCEL); - __sigdelset (&local_newmask, SIGSETXID); - set = &local_newmask; - } - return INLINE_SYSCALL_CALL (rt_sigprocmask, how, set, oset, _NSIG / 8); } libc_hidden_def (__sigprocmask)