Re: [musl] [PATCH v3 3/3] signal: add str2sig(3) from POSIX.1-2024

[Rich Felker <dalias@libc.org>]

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" <contact@hacktivis.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.

Rich