[PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

This avoids changing the entire sigset_t structure.  Updating the
actually used part is sufficient.

Tested on x86_64-linux-gnu and i686-linux-gnu.

-----
 signal/sigempty.c   | 7 +++----
 signal/sigfillset.c | 6 ++----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/signal/sigempty.c b/signal/sigempty.c
index 794e449997..e9f3b571b6 100644
--- a/signal/sigempty.c
+++ b/signal/sigempty.c
@@ -17,20 +17,19 @@
 
 #include <errno.h>
 #include <signal.h>
-#include <string.h>
+#include <sigsetops.h>
 
 /* Clear all signals from SET.  */
 int
 sigemptyset (sigset_t *set)
 {
-  if (set == NULL)
+  if (__glibc_unlikely (set == NULL))
     {
       __set_errno (EINVAL);
       return -1;
     }
 
-  memset (set, 0, sizeof (sigset_t));
-
+  __sigemptyset (set);
   return 0;
 }
 libc_hidden_def (sigemptyset)
diff --git a/signal/sigfillset.c b/signal/sigfillset.c
index 0ca8b6b534..29e98a5864 100644
--- a/signal/sigfillset.c
+++ b/signal/sigfillset.c
@@ -17,8 +17,8 @@
 
 #include <errno.h>
 #include <signal.h>
-#include <string.h>
 #include <internal-signals.h>
+#include <sigsetops.h>
 
 /* Set all signals in SET.  */
 int
@@ -30,10 +30,8 @@ sigfillset (sigset_t *set)
       return -1;
     }
 
-  memset (set, 0xff, sizeof (sigset_t));
-
+  __sigfillset (set);
   __clear_internal_signals (set);
-
   return 0;
 }
 libc_hidden_def (sigfillset)

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Andreas Schwab]

On Apr 22 2020, Florian Weimer wrote:

> This avoids changing the entire sigset_t structure.  Updating the
> actually used part is sufficient.

I'm not sure it's a good idea to leave most of the structure
uninitialized.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

* Andreas Schwab:

> On Apr 22 2020, Florian Weimer wrote:
>
>> This avoids changing the entire sigset_t structure.  Updating the
>> actually used part is sufficient.
>
> I'm not sure it's a good idea to leave most of the structure
> uninitialized.

Would you please elaborate?  How is this different from padding?

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Andreas Schwab]

On Apr 22 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Apr 22 2020, Florian Weimer wrote:
>>
>>> This avoids changing the entire sigset_t structure.  Updating the
>>> actually used part is sufficient.
>>
>> I'm not sure it's a good idea to leave most of the structure
>> uninitialized.
>
> Would you please elaborate?  How is this different from padding?

Padding is always unnamed.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

* Andreas Schwab:

> On Apr 22 2020, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> On Apr 22 2020, Florian Weimer wrote:
>>>
>>>> This avoids changing the entire sigset_t structure.  Updating the
>>>> actually used part is sufficient.
>>>
>>> I'm not sure it's a good idea to leave most of the structure
>>> uninitialized.
>>
>> Would you please elaborate?  How is this different from padding?
>
> Padding is always unnamed.

Not if it is called __glibc_reserved.  I obviously meant named
padding.

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

* Florian Weimer:

> * Andreas Schwab:
>
>> On Apr 22 2020, Florian Weimer wrote:
>>
>>> * Andreas Schwab:
>>>
>>>> On Apr 22 2020, Florian Weimer wrote:
>>>>
>>>>> This avoids changing the entire sigset_t structure.  Updating the
>>>>> actually used part is sufficient.
>>>>
>>>> I'm not sure it's a good idea to leave most of the structure
>>>> uninitialized.
>>>
>>> Would you please elaborate?  How is this different from padding?
>>
>> Padding is always unnamed.
>
> Not if it is called __glibc_reserved.  I obviously meant named
> padding.

And sigprocmask has always left the padding uninitialized (even before
Adhemerval's changes).

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Adhemerval Zanella]

On 22/04/2020 07:02, Florian Weimer wrote:
> * Florian Weimer:
> 
>> * Andreas Schwab:
>>
>>> On Apr 22 2020, Florian Weimer wrote:
>>>
>>>> * Andreas Schwab:
>>>>
>>>>> On Apr 22 2020, Florian Weimer wrote:
>>>>>
>>>>>> This avoids changing the entire sigset_t structure.  Updating the
>>>>>> actually used part is sufficient.
>>>>>
>>>>> I'm not sure it's a good idea to leave most of the structure
>>>>> uninitialized.
>>>>
>>>> Would you please elaborate?  How is this different from padding?
>>>
>>> Padding is always unnamed.
>>
>> Not if it is called __glibc_reserved.  I obviously meant named
>> padding.
> 
> And sigprocmask has always left the padding uninitialized (even before
> Adhemerval's changes).
> 

Should we change __sigemptyset/__sigfillset to fill the ununsed bits
instead?

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> On 22/04/2020 07:02, Florian Weimer wrote:
>> * Florian Weimer:
>> 
>>> * Andreas Schwab:
>>>
>>>> On Apr 22 2020, Florian Weimer wrote:
>>>>
>>>>> * Andreas Schwab:
>>>>>
>>>>>> On Apr 22 2020, Florian Weimer wrote:
>>>>>>
>>>>>>> This avoids changing the entire sigset_t structure.  Updating the
>>>>>>> actually used part is sufficient.
>>>>>>
>>>>>> I'm not sure it's a good idea to leave most of the structure
>>>>>> uninitialized.
>>>>>
>>>>> Would you please elaborate?  How is this different from padding?
>>>>
>>>> Padding is always unnamed.
>>>
>>> Not if it is called __glibc_reserved.  I obviously meant named
>>> padding.
>> 
>> And sigprocmask has always left the padding uninitialized (even before
>> Adhemerval's changes).
>
> Should we change __sigemptyset/__sigfillset to fill the ununsed bits
> instead?

We would have to change sigprocmask as well.

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

* Florian Weimer:

> * Adhemerval Zanella via Libc-alpha:
>
>> On 22/04/2020 07:02, Florian Weimer wrote:
>>> * Florian Weimer:
>>> 
>>>> * Andreas Schwab:
>>>>
>>>>> On Apr 22 2020, Florian Weimer wrote:
>>>>>
>>>>>> * Andreas Schwab:
>>>>>>
>>>>>>> On Apr 22 2020, Florian Weimer wrote:
>>>>>>>
>>>>>>>> This avoids changing the entire sigset_t structure.  Updating the
>>>>>>>> actually used part is sufficient.
>>>>>>>
>>>>>>> I'm not sure it's a good idea to leave most of the structure
>>>>>>> uninitialized.
>>>>>>
>>>>>> Would you please elaborate?  How is this different from padding?
>>>>>
>>>>> Padding is always unnamed.
>>>>
>>>> Not if it is called __glibc_reserved.  I obviously meant named
>>>> padding.
>>> 
>>> And sigprocmask has always left the padding uninitialized (even before
>>> Adhemerval's changes).
>>
>> Should we change __sigemptyset/__sigfillset to fill the ununsed bits
>> instead?
>
> We would have to change sigprocmask as well.

Just to be clear: I don't think writing more data in sigprocmask is a
useful change.  We might even have to introduce a new symbol version.

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Adhemerval Zanella]

On 22/04/2020 09:22, Florian Weimer wrote:
> * Florian Weimer:
> 
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> On 22/04/2020 07:02, Florian Weimer wrote:
>>>> * Florian Weimer:
>>>>
>>>>> * Andreas Schwab:
>>>>>
>>>>>> On Apr 22 2020, Florian Weimer wrote:
>>>>>>
>>>>>>> * Andreas Schwab:
>>>>>>>
>>>>>>>> On Apr 22 2020, Florian Weimer wrote:
>>>>>>>>
>>>>>>>>> This avoids changing the entire sigset_t structure.  Updating the
>>>>>>>>> actually used part is sufficient.
>>>>>>>>
>>>>>>>> I'm not sure it's a good idea to leave most of the structure
>>>>>>>> uninitialized.
>>>>>>>
>>>>>>> Would you please elaborate?  How is this different from padding?
>>>>>>
>>>>>> Padding is always unnamed.
>>>>>
>>>>> Not if it is called __glibc_reserved.  I obviously meant named
>>>>> padding.
>>>>
>>>> And sigprocmask has always left the padding uninitialized (even before
>>>> Adhemerval's changes).
>>>
>>> Should we change __sigemptyset/__sigfillset to fill the ununsed bits
>>> instead?
>>
>> We would have to change sigprocmask as well.
> 
> Just to be clear: I don't think writing more data in sigprocmask is a
> useful change.  We might even have to introduce a new symbol version.
> 

Although I considered this option on BZ#25657, I tend to agree with you.
The data is no longer accessed by GNU signal functions, so it should not 
trigger any static or sanitizer.

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

Any additional comments on this patch?  Thanks.

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Adhemerval Zanella]

On 28/04/2020 07:48, Florian Weimer wrote:
> Any additional comments on this patch?  Thanks.
> 

LGTM, thanks.

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

* Andreas Schwab:

> On Apr 22 2020, Florian Weimer wrote:
>
>> This avoids changing the entire sigset_t structure.  Updating the
>> actually used part is sufficient.
>
> I'm not sure it's a good idea to leave most of the structure
> uninitialized.

Andreas, is this a sustained objection?  Thanks.

Re: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> On 28/04/2020 07:48, Florian Weimer wrote:
>> Any additional comments on this patch?  Thanks.
>> 
>
> LGTM, thanks.

Thanks, I pushed this without the spurious __glibc_unlikely change.