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

[Rich Felker <dalias@libc.org>]

On Mon, Aug 05, 2024 at 08:56:07AM +0200, contact@hacktivis.me wrote:
> From: "Haelwenn (lanodan) Monnier" <contact@hacktivis.me>
> 
> ---
>  include/signal.h     |  1 +
>  src/signal/str2sig.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 src/signal/str2sig.c
> 
> diff --git a/include/signal.h b/include/signal.h
> index 217cfa08..fd486cd0 100644
> --- a/include/signal.h
> +++ b/include/signal.h
> @@ -235,6 +235,7 @@ void psignal(int, const char *);
>  
>  #define SIG2STR_MAX sizeof("RTMIN+32")
>  int sig2str(int signum, char *str);
> +int str2sig(const char *restrict str, int *restrict pnum);
>  
>  #endif
>  
> diff --git a/src/signal/str2sig.c b/src/signal/str2sig.c
> new file mode 100644
> index 00000000..b6a4fc23
> --- /dev/null
> +++ b/src/signal/str2sig.c
> @@ -0,0 +1,45 @@
> +#include <signal.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <stdlib.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);
> +
> +	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);
> +
> +	// signal aliases
> +	if (strcmp(str, "IOT") == 0)
> +		return (*pnum = SIGIOT, 0);
> +	if (strcmp(str, "UNUSED") == 0)
> +		return (*pnum = SIGUNUSED, 0);

If there were a lot of these I'd lean towards some table to avoid the
above, but there seem to be only two that fit this pattern, so what
you've done is probably best.

> +#if SIGPOLL != SIGIO
> +	if (strcmp(str, "POLL") == 0)
> +		return (*pnum = SIGPOLL, 0);
> +#endif

I think it's when they're equal, not when they're not-equal, that you
need to special-case this. Otherwise "POLL" won't be accepted on archs
where the values are equal. Of course you'd need to special-case both
if it weren't in the table, but see my comment on sig2str about
putting POLL in the table when not-equal.

> +	if (strcmp(str, "RTMIN") == 0)
> +		return (*pnum = SIGRTMIN, 0);
> +	if (strcmp(str, "RTMAX") == 0)
> +		return (*pnum = SIGRTMAX, 0);

These don't count in the "only two" I mentioned above because the
results are not a constant translation but runtime-variable.

This does raise a point that we could have SIGRTMIN and SIGRTMAX
expand to constants at musl build time, since the only way they could
change is by changing musl. My leaning is kinda not to do that,
though, because I've toyed with the idea of picking the
implementation-internal signals dynamically so that nesting in
qemu-user with some signals already stolen by the outer implementation
can work. I'm not sure if that's actually practical (it's more of a
pain than it sounds like, at least) but it's probably best to leave it
the way you've done it so as not to lock that possibility out.

> +	if (strncmp(str, "RTMIN+", 6) == 0 || strncmp(str, "RTMAX-", 6) == 0)
> +	{
> +		errno = 0;
> +		long sigrt = strtol(str+6, NULL, 10);
> +		if(errno != 0 || sigrt < 1 || sigrt > SIGRTMAX - SIGRTMIN) return -1;
> +
> +		*pnum = str[5] == '+' ? SIGRTMIN + sigrt : SIGRTMAX - sigrt;
> +		return 0;
> +	}
> +
> +	return -1;
> +}

The call to strtol isn't validating that there's not additional junk
after the digits, or that there's not a second sign character or
whitespace before the digits.

The canonical way to solve this is checking isdigit on the first char
and using the pend argument to see where parsing stopped, but in this
case, since the + or - is mandatory and already checked, you could
pass str+5 instead of str+6 to have it parse the + or - too. This
would require changing the arithmetic that follows, It also
complicates accepting 0 if you want to, but the current code rejects
0, so maybe that's okay?

It's also not clear if you want to accept over-long inputs like
RTMIN+000001. If not, checking that first char after + or - is in
range '1' to '9' would probably be the easiest.

Another possibility to consider is that interfacing with strtol is
sufficiently complicated that it's not worth bothering, and that
open-coding a-'0' or 10*(a-'0')+(b-'0') is easier (if you don't
actually intend to accept over-length representations).

Rich