From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 3FBBE21453 for ; Sun, 11 Aug 2024 02:43:29 +0200 (CEST) Received: (qmail 5712 invoked by uid 550); 11 Aug 2024 00:43:23 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 5673 invoked from network); 11 Aug 2024 00:43:22 -0000 Date: Sat, 10 Aug 2024 20:43:15 -0400 From: Rich Felker To: contact@hacktivis.me Cc: musl@lists.openwall.com Message-ID: <20240811004314.GI10433@brightrain.aerifal.cx> References: <20240809153423.30829-1-contact@hacktivis.me> <20240809153423.30829-3-contact@hacktivis.me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240809153423.30829-3-contact@hacktivis.me> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH v3 3/3] signal: add str2sig(3) from POSIX.1-2024 A few things I didn't catch before: On Fri, Aug 09, 2024 at 05:34:23PM +0200, contact@hacktivis.me wrote: > From: "Haelwenn (lanodan) Monnier" > > --- > 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 > +#include > +#include > +#include > +#include > + > +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. Rich