mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] Bug in src/signal/block.c
@ 2021-07-28 15:00 Jasper Hugunin
  2021-07-28 15:53 ` Rich Felker
  0 siblings, 1 reply; 8+ messages in thread
From: Jasper Hugunin @ 2021-07-28 15:00 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

Hello,

In musl, as far as I can tell, `_NSIG` is always defined as either 65, or
128 (for all three MIPS architectures) at the bottom of
`${arch}/bits/signal.h`. Meanwhile, in `src/signal/block.c`, there is a
test `#if ULONG_MAX == 0xffffffff && _NSIG == 129`, which will never
succeed since _NSIG will be 128 instead of 129. This seems likely to be
left over from Commit: fix _NSIG and SIGRTMAX on mips
<https://git.musl-libc.org/cgit/musl/commit/arch?id=7c440977db9444d7e6b1c3dcb1fdf4ee49ca4158>
.

I have not demonstrated the bug, I found it by inspection of the source. My
guess is that this bug causes __block_all_sigs to fail to block high real
time signals on MIPS. At best, however, this test seems to be dead code.

(I am not subscribed to the mailing list; please cc me directly on any
responses I need to see.)
My apologies if I have misunderstood the situation.

Sincerely,
- Jasper Hugunin

[-- Attachment #2: Type: text/html, Size: 1063 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Bug in src/signal/block.c
  2021-07-28 15:00 [musl] Bug in src/signal/block.c Jasper Hugunin
@ 2021-07-28 15:53 ` Rich Felker
  2021-07-28 17:11   ` Laurent Bercot
  2021-07-28 19:36   ` Rich Felker
  0 siblings, 2 replies; 8+ messages in thread
From: Rich Felker @ 2021-07-28 15:53 UTC (permalink / raw)
  To: Jasper Hugunin; +Cc: musl

On Wed, Jul 28, 2021 at 08:00:00AM -0700, Jasper Hugunin wrote:
> Hello,
> 
> In musl, as far as I can tell, `_NSIG` is always defined as either 65, or
> 128 (for all three MIPS architectures) at the bottom of
> `${arch}/bits/signal.h`. Meanwhile, in `src/signal/block.c`, there is a
> test `#if ULONG_MAX == 0xffffffff && _NSIG == 129`, which will never
> succeed since _NSIG will be 128 instead of 129. This seems likely to be
> left over from Commit: fix _NSIG and SIGRTMAX on mips
> <https://git.musl-libc.org/cgit/musl/commit/arch?id=7c440977db9444d7e6b1c3dcb1fdf4ee49ca4158>
> ..
> 
> I have not demonstrated the bug, I found it by inspection of the source. My
> guess is that this bug causes __block_all_sigs to fail to block high real
> time signals on MIPS. At best, however, this test seems to be dead code.
> 
> (I am not subscribed to the mailing list; please cc me directly on any
> responses I need to see.)
> My apologies if I have misunderstood the situation.

Thanks! This is a real bug that will prevent signal blocking from
working correctly on mips, resulting in application code being able to
run in contexts where it is unsafe for that to happen if the
application installs signal handlers on high signal numbers.

Rich

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Bug in src/signal/block.c
  2021-07-28 15:53 ` Rich Felker
@ 2021-07-28 17:11   ` Laurent Bercot
  2021-07-28 18:43     ` Michael Forney
  2021-07-28 19:30     ` Rich Felker
  2021-07-28 19:36   ` Rich Felker
  1 sibling, 2 replies; 8+ messages in thread
From: Laurent Bercot @ 2021-07-28 17:11 UTC (permalink / raw)
  To: musl

>>  succeed since _NSIG will be 128 instead of 129.

  I happen to be in the process of updating my programming library
performing workarounds for badly-specified parts of POSIX and related
functions.

  NSIG is one of those parts. It is not specified by POSIX, but it is
useful to have a walkable (as in, not 8*sizeof(sigset_t)) upper bound
for the number of signals on a system.

  But NSIG is badly specified even across the systems where it exists.
On glibc, it is 1 + the highest signal number. On FreeBSD and OpenBSD
at least, it is the highest signal number.

  musl appears to align on glibc; I suppose the value for MIPS will be
updated to 129, for consistency.

  Can I assume that if NSIG is defined, it is strictly greater than the
highest signal number, except on the BSDs which are an unfortunate
exception, as they too often are?

--
  Laurent


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Bug in src/signal/block.c
  2021-07-28 17:11   ` Laurent Bercot
@ 2021-07-28 18:43     ` Michael Forney
  2021-07-28 19:30     ` Rich Felker
  1 sibling, 0 replies; 8+ messages in thread
From: Michael Forney @ 2021-07-28 18:43 UTC (permalink / raw)
  To: musl

"Laurent Bercot" <ska-dietlibc@skarnet.org> wrote:
>   I happen to be in the process of updating my programming library
> performing workarounds for badly-specified parts of POSIX and related
> functions.
> 
>   NSIG is one of those parts. It is not specified by POSIX, but it is
> useful to have a walkable (as in, not 8*sizeof(sigset_t)) upper bound
> for the number of signals on a system.
>
>   But NSIG is badly specified even across the systems where it exists.
> On glibc, it is 1 + the highest signal number. On FreeBSD and OpenBSD
> at least, it is the highest signal number.

The current draft of POSIX issue 8 includes a resolution for
https://www.austingroupbugs.net/view.php?id=741 (Add a NSIG constant
(or, alternatively, SIGMAX))

However, rather than adding NSIG, they introduced a new sysconf
variable _SC_NSIG, defined as the highest supported signal number
+ 1, and NSIG_MAX, which is defined as follows:

  Maximum possible return value of sysconf(_SC_NSIG). See XSH sysconf().
  The value of {NSIG_MAX} shall be no greater than the number of
  signals that the sigset_t type (see <signal.h>) is capable of
  representing, ignoring any restrictions imposed by sigfillset() or
  sigaddset().

They probably went with this route instead of specifying NSIG due
to those inconsistencies you mentioned. Unfortunately, as far as
I'm aware, both _SC_NSIG and NSIG_MAX are not yet present in any
libc.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Bug in src/signal/block.c
  2021-07-28 17:11   ` Laurent Bercot
  2021-07-28 18:43     ` Michael Forney
@ 2021-07-28 19:30     ` Rich Felker
  1 sibling, 0 replies; 8+ messages in thread
From: Rich Felker @ 2021-07-28 19:30 UTC (permalink / raw)
  To: Laurent Bercot; +Cc: musl

On Wed, Jul 28, 2021 at 05:11:11PM +0000, Laurent Bercot wrote:
> >> succeed since _NSIG will be 128 instead of 129.
> 
>  I happen to be in the process of updating my programming library
> performing workarounds for badly-specified parts of POSIX and related
> functions.
> 
>  NSIG is one of those parts. It is not specified by POSIX, but it is
> useful to have a walkable (as in, not 8*sizeof(sigset_t)) upper bound
> for the number of signals on a system.

I thought it was going to be POSIX-future, but maybe they did the ugly
_SC_NSIG-only approach instead (which partly defeats the purpose,
although you can use 8*sizeof(sigset_t) as an ugly upper bound on
NSIG where you need a constant expression).

>  But NSIG is badly specified even across the systems where it exists.
> On glibc, it is 1 + the highest signal number. On FreeBSD and OpenBSD
> at least, it is the highest signal number.

That's unfortunate.

>  musl appears to align on glibc; I suppose the value for MIPS will be
> updated to 129, for consistency.

No. The greatest signal number on mips is 127, not 128. There's a long
story behind this and it involves the kernel doing stupid stuff.
Inspect sys/wait.h if you want to try to figure it out yourself. :-)

Rich

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Bug in src/signal/block.c
  2021-07-28 15:53 ` Rich Felker
  2021-07-28 17:11   ` Laurent Bercot
@ 2021-07-28 19:36   ` Rich Felker
  2021-07-28 19:52     ` Jasper Hugunin
  1 sibling, 1 reply; 8+ messages in thread
From: Rich Felker @ 2021-07-28 19:36 UTC (permalink / raw)
  To: Jasper Hugunin; +Cc: musl

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

On Wed, Jul 28, 2021 at 11:53:41AM -0400, Rich Felker wrote:
> On Wed, Jul 28, 2021 at 08:00:00AM -0700, Jasper Hugunin wrote:
> > Hello,
> > 
> > In musl, as far as I can tell, `_NSIG` is always defined as either 65, or
> > 128 (for all three MIPS architectures) at the bottom of
> > `${arch}/bits/signal.h`. Meanwhile, in `src/signal/block.c`, there is a
> > test `#if ULONG_MAX == 0xffffffff && _NSIG == 129`, which will never
> > succeed since _NSIG will be 128 instead of 129. This seems likely to be
> > left over from Commit: fix _NSIG and SIGRTMAX on mips
> > <https://git.musl-libc.org/cgit/musl/commit/arch?id=7c440977db9444d7e6b1c3dcb1fdf4ee49ca4158>
> > ..
> > 
> > I have not demonstrated the bug, I found it by inspection of the source. My
> > guess is that this bug causes __block_all_sigs to fail to block high real
> > time signals on MIPS. At best, however, this test seems to be dead code.
> > 
> > (I am not subscribed to the mailing list; please cc me directly on any
> > responses I need to see.)
> > My apologies if I have misunderstood the situation.
> 
> Thanks! This is a real bug that will prevent signal blocking from
> working correctly on mips, resulting in application code being able to
> run in contexts where it is unsafe for that to happen if the
> application installs signal handlers on high signal numbers.

Does the attached patch look ok?

Rich

[-- Attachment #2: sigblock-mips.diff --]
[-- Type: text/plain, Size: 433 bytes --]

diff --git a/src/signal/block.c b/src/signal/block.c
index d7f61001..cc8698f0 100644
--- a/src/signal/block.c
+++ b/src/signal/block.c
@@ -3,9 +3,9 @@
 #include <signal.h>
 
 static const unsigned long all_mask[] = {
-#if ULONG_MAX == 0xffffffff && _NSIG == 129
+#if ULONG_MAX == 0xffffffff && _NSIG > 65
 	-1UL, -1UL, -1UL, -1UL
-#elif ULONG_MAX == 0xffffffff
+#elif ULONG_MAX == 0xffffffff || _NSIG > 65
 	-1UL, -1UL
 #else
 	-1UL

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Bug in src/signal/block.c
  2021-07-28 19:36   ` Rich Felker
@ 2021-07-28 19:52     ` Jasper Hugunin
  2021-07-28 22:04       ` Rich Felker
  0 siblings, 1 reply; 8+ messages in thread
From: Jasper Hugunin @ 2021-07-28 19:52 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

[-- Attachment #1: Type: text/plain, Size: 1966 bytes --]

It looks ok to me, I think that will work for all the architectures
currently around.

I do worry about what would happen on an architecture setting `_NSIG` to
something other than 65 or 128, say 256 or 230 or something like that, but
that is a larger scale concern for the whole musl codebase.

(Note: I am not an expert on signal handling, I was just poking around the
code when I spotted this inconsistency.)

Sincerely,
- Jasper Hugunin

On Wed, Jul 28, 2021 at 12:36 PM Rich Felker <dalias@libc.org> wrote:

> On Wed, Jul 28, 2021 at 11:53:41AM -0400, Rich Felker wrote:
> > On Wed, Jul 28, 2021 at 08:00:00AM -0700, Jasper Hugunin wrote:
> > > Hello,
> > >
> > > In musl, as far as I can tell, `_NSIG` is always defined as either 65,
> or
> > > 128 (for all three MIPS architectures) at the bottom of
> > > `${arch}/bits/signal.h`. Meanwhile, in `src/signal/block.c`, there is a
> > > test `#if ULONG_MAX == 0xffffffff && _NSIG == 129`, which will never
> > > succeed since _NSIG will be 128 instead of 129. This seems likely to be
> > > left over from Commit: fix _NSIG and SIGRTMAX on mips
> > > <
> https://git.musl-libc.org/cgit/musl/commit/arch?id=7c440977db9444d7e6b1c3dcb1fdf4ee49ca4158
> >
> > > ..
> > >
> > > I have not demonstrated the bug, I found it by inspection of the
> source. My
> > > guess is that this bug causes __block_all_sigs to fail to block high
> real
> > > time signals on MIPS. At best, however, this test seems to be dead
> code.
> > >
> > > (I am not subscribed to the mailing list; please cc me directly on any
> > > responses I need to see.)
> > > My apologies if I have misunderstood the situation.
> >
> > Thanks! This is a real bug that will prevent signal blocking from
> > working correctly on mips, resulting in application code being able to
> > run in contexts where it is unsafe for that to happen if the
> > application installs signal handlers on high signal numbers.
>
> Does the attached patch look ok?
>
> Rich
>

[-- Attachment #2: Type: text/html, Size: 2680 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Bug in src/signal/block.c
  2021-07-28 19:52     ` Jasper Hugunin
@ 2021-07-28 22:04       ` Rich Felker
  0 siblings, 0 replies; 8+ messages in thread
From: Rich Felker @ 2021-07-28 22:04 UTC (permalink / raw)
  To: Jasper Hugunin; +Cc: musl

On Wed, Jul 28, 2021 at 12:52:23PM -0700, Jasper Hugunin wrote:
> It looks ok to me, I think that will work for all the architectures
> currently around.
> 
> I do worry about what would happen on an architecture setting `_NSIG` to
> something other than 65 or 128, say 256 or 230 or something like that, but
> that is a larger scale concern for the whole musl codebase.
> 
> (Note: I am not an expert on signal handling, I was just poking around the
> code when I spotted this inconsistency.)

Thanks for your review. It's a historical mistake that MIPS had 128
(later changed to 127) signals rather than the 64 everywhere else. It
would be very unexpected for any new arch to deviate like this, since
nowadays everyone realizes trying to mimic the old proprietary unices
that once existed 'natively' on each arch was a mistake. If it does
happen, generalizing to support it would be part of the work of adding
that arch.

Rich


> On Wed, Jul 28, 2021 at 12:36 PM Rich Felker <dalias@libc.org> wrote:
> 
> > On Wed, Jul 28, 2021 at 11:53:41AM -0400, Rich Felker wrote:
> > > On Wed, Jul 28, 2021 at 08:00:00AM -0700, Jasper Hugunin wrote:
> > > > Hello,
> > > >
> > > > In musl, as far as I can tell, `_NSIG` is always defined as either 65,
> > or
> > > > 128 (for all three MIPS architectures) at the bottom of
> > > > `${arch}/bits/signal.h`. Meanwhile, in `src/signal/block.c`, there is a
> > > > test `#if ULONG_MAX == 0xffffffff && _NSIG == 129`, which will never
> > > > succeed since _NSIG will be 128 instead of 129. This seems likely to be
> > > > left over from Commit: fix _NSIG and SIGRTMAX on mips
> > > > <
> > https://git.musl-libc.org/cgit/musl/commit/arch?id=7c440977db9444d7e6b1c3dcb1fdf4ee49ca4158
> > >
> > > > ..
> > > >
> > > > I have not demonstrated the bug, I found it by inspection of the
> > source. My
> > > > guess is that this bug causes __block_all_sigs to fail to block high
> > real
> > > > time signals on MIPS. At best, however, this test seems to be dead
> > code.
> > > >
> > > > (I am not subscribed to the mailing list; please cc me directly on any
> > > > responses I need to see.)
> > > > My apologies if I have misunderstood the situation.
> > >
> > > Thanks! This is a real bug that will prevent signal blocking from
> > > working correctly on mips, resulting in application code being able to
> > > run in contexts where it is unsafe for that to happen if the
> > > application installs signal handlers on high signal numbers.
> >
> > Does the attached patch look ok?
> >
> > Rich
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-07-28 22:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-28 15:00 [musl] Bug in src/signal/block.c Jasper Hugunin
2021-07-28 15:53 ` Rich Felker
2021-07-28 17:11   ` Laurent Bercot
2021-07-28 18:43     ` Michael Forney
2021-07-28 19:30     ` Rich Felker
2021-07-28 19:36   ` Rich Felker
2021-07-28 19:52     ` Jasper Hugunin
2021-07-28 22:04       ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).