[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240811025424.GJ10433@brightrain.aerifal.cx>
Date: Sat, 10 Aug 2024 22:54:24 -0400
From: Rich Felker <dalias@...c.org>
To: contact@...ktivis.me
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH v3 3/3] signal: add str2sig(3) from POSIX.1-2024
On Sat, Aug 10, 2024 at 08:43:15PM -0400, Rich Felker wrote:
> A few things I didn't catch before:
>
> On Fri, Aug 09, 2024 at 05:34:23PM +0200, contact@...ktivis.me wrote:
> > From: "Haelwenn (lanodan) Monnier" <contact@...ktivis.me>
> >
> > ---
> > include/signal.h | 1 +
> > src/signal/str2sig.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 55 insertions(+)
> > create mode 100644 src/signal/str2sig.c
> >
> > diff --git a/include/signal.h b/include/signal.h
> > index 94ac29b1..5451424d 100644
> > --- a/include/signal.h
> > +++ b/include/signal.h
> > @@ -237,6 +237,7 @@ void psignal(int, const char *);
> > // Bumped to 13 to be safe if a case like "SIGRTMIN+nnn" happens
> > #define SIG2STR_MAX 13
> > int sig2str(int signum, char *str);
> > +int str2sig(const char *__restrict str, int *__restrict pnum);
>
> As a matter of conformance, declarations in public headers can't have
> non-reserved identifiers like str or pnum in them (these might already
> have been #defined'd by the application). As a matter of policy, we
> just don't use argument names or other "creative" content (including
> comments) in the public headers at all, mainly because the headers
> have been intended to be statements of fact not subject to copyright
> (so a dynamic program using them is not a derived work and doesn't
> rely on license at all).
>
> (There might be a few places where that's not entirely followed, but
> if so they should be fixed.)
>
> > #endif
> >
> > diff --git a/src/signal/str2sig.c b/src/signal/str2sig.c
> > new file mode 100644
> > index 00000000..e4c17c57
> > --- /dev/null
> > +++ b/src/signal/str2sig.c
> > @@ -0,0 +1,54 @@
> > +#include <signal.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <stdlib.h>
> > +#include <ctype.h>
> > +
> > +int str2sig(const char *restrict str, int *restrict pnum)
> > +{
> > + if (str[0] == '\0') return -1;
> > +
> > + errno = 0;
> > + long signum = strtol(str, NULL, 10);
> > + if (errno == 0 && signum < _NSIG) return (*pnum = signum, 0);
>
> I don't think the errno check is either necessary or sufficient. As
> written, this code won't reject something like "12x". Generally, you
> need to pass a pend argument, like:
>
> char *end;
> long signum = strtol(str, &end, 10);
> if (!*end && signum < _NSIG) ...
>
> which checks that the whole input was consumed, and thereby also that
> there was no error except possible range overflow (which would be
> caught by range checks).
>
> However there's also a missing range check for negative values. This
> could probably be handled by use of an unsigned type here, but unless
> the intent is to accept inputs starting with + or -, the entire strtol
> path should probably only be attempted if (isdigit(str[0])), in which
> case negative can't happen.
>
> > + if (strnlen(str, sizeof *__sys_signame) <= sizeof *__sys_signame)
> > + for (int i = 0; i < sizeof __sys_signame/sizeof *__sys_signame; i++)
> > + if (strncmp(str, __sys_signame[i], sizeof *__sys_signame) == 0)
> > + return (*pnum = i, 0);
>
> I think the indexing for __sys_signame could be adjusted by 1 so
> there's not an unused slot 0. The entries could also be 6-byte rather
> than 7-byte.
>
> > + // signal aliases
> > + if (strcmp(str, "IOT") == 0)
> > + return (*pnum = SIGIOT, 0);
> > + if (strcmp(str, "UNUSED") == 0)
> > + return (*pnum = SIGUNUSED, 0);
> > +#if SIGPOLL == SIGIO
> > + if (strcmp(str, "POLL") == 0)
> > + return (*pnum = SIGPOLL, 0);
> > +#endif
> > +
> > + if (strcmp(str, "RTMIN") == 0)
> > + return (*pnum = SIGRTMIN, 0);
> > + if (strcmp(str, "RTMAX") == 0)
> > + return (*pnum = SIGRTMAX, 0);
> > +
> > + if (strncmp(str, "RTMIN+", 6) == 0 || strncmp(str, "RTMAX-", 6) == 0)
> > + {
> > + if(!isdigit(str[6])) return -1;
> > +
> > + int sigrt = str[6]-'0';
> > +
> > + if(str[7] != '\0')
> > + {
> > + if(!isdigit(str[7])) return -1;
> > +
> > + sigrt *= 10;
> > + sigrt += str[7]-'0';
> > + }
>
> At this point you're missing a check that there's not further junk (or
> more digits) after the second one. It could be something like:
>
> + if(!isdigit(str[7]) || str[8]) return -1;
>
> This isn't highly important, but the style str[8] vs str[8]!='\0' is
> generally preferred in musl.
OK, I've finally gone and dug up and read the specification:
https://pubs.opengroup.org/onlinepubs/9799919799/functions/sig2str.html
and it requires:
"If str points to a string of the form "RTMIN+n", where n is a
decimal representation of a number between 1 and
SIGRTMAX-SIGRTMIN-1 inclusive, the stored value shall be equal to
SIGRTMIN+n.
"If str points to a string of the form "RTMAX-n", where n is a
decimal representation of a number between 1 and
SIGRTMAX-SIGRTMIN-1 inclusive, the stored value shall be equal to
SIGRTMAX-n."
So there is no requirement that the decimal number be in shortest
form. As such this needs a loop or strtol (after checking isdigit).
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
