[v2,5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask
diff mbox series

Message ID 20191210183221.26912-5-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [v3,1/7] Fix __libc_signal_block_all on sparc64
Related show

Commit Message

Adhemerval Zanella Dec. 10, 2019, 6:32 p.m. UTC
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).

Checked on x86_64-linux-gnu.
---
 sysdeps/unix/sysv/linux/sigprocmask.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

Comments

Florian Weimer Dec. 12, 2019, 12:54 p.m. UTC | #1
* 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
Adhemerval Zanella Dec. 12, 2019, 1:01 p.m. UTC | #2
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.
Florian Weimer Dec. 12, 2019, 1:12 p.m. UTC | #3
* 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
Adhemerval Zanella Dec. 12, 2019, 1:43 p.m. UTC | #4
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.
Zack Weinberg Dec. 12, 2019, 5:59 p.m. UTC | #5
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
Adhemerval Zanella Dec. 12, 2019, 7:05 p.m. UTC | #6
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.
Adhemerval Zanella Dec. 12, 2019, 7:07 p.m. UTC | #7
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?
Zack Weinberg Dec. 12, 2019, 7:29 p.m. UTC | #8
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
Adhemerval Zanella Dec. 12, 2019, 7:43 p.m. UTC | #9
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).

Patch
diff mbox series

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)