| Message ID | 87eesglyc9.fsf@mid.deneb.enyo.de |
|---|---|
| State | Committed |
| Headers |
Return-Path: <fw@deneb.enyo.de> X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from albireo.enyo.de (albireo.enyo.de [37.24.231.21]) by sourceware.org (Postfix) with ESMTPS id 590E0386F43F for <libc-alpha@sourceware.org>; Wed, 22 Apr 2020 08:27:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 590E0386F43F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=deneb.enyo.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=fw@deneb.enyo.de Received: from [172.17.203.2] (helo=deneb.enyo.de) by albireo.enyo.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) id 1jRAj0-0007NQ-64 for libc-alpha@sourceware.org; Wed, 22 Apr 2020 08:27:18 +0000 Received: from fw by deneb.enyo.de with local (Exim 4.92) (envelope-from <fw@deneb.enyo.de>) id 1jRAj0-0007D6-21 for libc-alpha@sourceware.org; Wed, 22 Apr 2020 10:27:18 +0200 From: Florian Weimer <fw@deneb.enyo.de> To: libc-alpha@sourceware.org Subject: [PATCH] signal: Use <sigsetops.h> for sigemptyset, sigfillset Date: Wed, 22 Apr 2020 10:27:18 +0200 Message-ID: <87eesglyc9.fsf@mid.deneb.enyo.de> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-21.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <http://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <http://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> X-List-Received-Date: Wed, 22 Apr 2020 08:27:20 -0000 |
| Series |
signal: Use <sigsetops.h> for sigemptyset, sigfillset
|
|
Commit Message
Florian Weimer
April 22, 2020, 8:27 a.m. UTC
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(-)
Comments
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: > 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?
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: > 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.
* 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).
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?
* 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.
* 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.
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.
* 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.
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)