[PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Adhemerval Zanella-2


On 12/12/2019 13:30, Andreas Schwab wrote:

> On Dez 12 2019, Adhemerval Zanella wrote:
>
>> Well I am seeing such behaviour with gcc 9.2.1 on x86_64 at least:
>
> That's a missed optimisation bug in gcc then.  There should not be a
> difference between a const compound literal and a static const object,
> if only constant expressions are used for initialisation.
>
> Andreas.
>

Yes, but it does not answer my previous question: is this a block for
the patch?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Andreas Schwab
On Dez 12 2019, Adhemerval Zanella wrote:

> Yes, but it does not answer my previous question: is this a block for
> the patch?

If that doesn't help fixing the bug then be it.  But a gcc bug would be
in order anyway.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Adhemerval Zanella-2


On 12/12/2019 13:53, Andreas Schwab wrote:
> On Dez 12 2019, Adhemerval Zanella wrote:
>
>> Yes, but it does not answer my previous question: is this a block for
>> the patch?
>
> If that doesn't help fixing the bug then be it.  But a gcc bug would be
> in order anyway.

I will open a gcc bug to track it, is this patch ok with an associated
bug report on gcc?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Andreas Schwab
Perhaps add a comment why a compound literal cannot be used.  Ok with
that change.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Adhemerval Zanella-2


On 12/12/2019 14:00, Andreas Schwab wrote:
> Perhaps add a comment why a compound literal cannot be used.  Ok with
> that change.
>

Ack, I will do it.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Zack Weinberg-2
In reply to this post by Adhemerval Zanella-2
On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella
<[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Joseph Myers
In reply to this post by Andreas Schwab
On Thu, 12 Dec 2019, Andreas Schwab wrote:

> On Dez 12 2019, Adhemerval Zanella wrote:
>
> > Well I am seeing such behaviour with gcc 9.2.1 on x86_64 at least:
>
> That's a missed optimisation bug in gcc then.  There should not be a
> difference between a const compound literal and a static const object,
> if only constant expressions are used for initialisation.

There's a note at the bottom of https://gcc.gnu.org/c99status.html 
specifically about this ("const-qualified compound literals could share
storage with each other and with string literals, but currently don't.").

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Adhemerval Zanella-2
In reply to this post by Zack Weinberg-2


On 12/12/2019 14:59, Zack Weinberg wrote:

> On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella
> <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Adhemerval Zanella-2


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
>> <[hidden email]> 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?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Zack Weinberg-2
In reply to this post by Adhemerval Zanella-2
On Thu, Dec 12, 2019 at 2:05 PM Adhemerval Zanella
<[hidden email]> wrote:

> On 12/12/2019 14:59, Zack Weinberg wrote:
> > On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella
> > <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Adhemerval Zanella-2


On 12/12/2019 16:29, Zack Weinberg wrote:

> On Thu, Dec 12, 2019 at 2:05 PM Adhemerval Zanella
> <[hidden email]> wrote:
>> On 12/12/2019 14:59, Zack Weinberg wrote:
>>> On Thu, Dec 12, 2019 at 8:44 AM Adhemerval Zanella
>>> <[hidden email]> 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).
12