Re: git: 2e0caa7c7e14 - main - libutil: Really fix expand_number(3)

On 8/2/25 10:06, Dag-Erling Smørgrav wrote:
> The branch main has been updated by des:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=2e0caa7c7e14d7bdc89ec43be9bc848abe1ca264
> 
> commit 2e0caa7c7e14d7bdc89ec43be9bc848abe1ca264
> Author:     Dag-Erling Smørgrav <des@FreeBSD.org>
> AuthorDate: 2025-08-02 14:05:36 +0000
> Commit:     Dag-Erling Smørgrav <des@FreeBSD.org>
> CommitDate: 2025-08-02 14:05:36 +0000
> 
>      libutil: Really fix expand_number(3)
>      
>      It is unclear whether this function was originally intended to support
>      negative numbers.  The original implementation used signed integers,
>      but actually giving it a negative number as input would have invoked
>      undefined behavior, and the comments (since removed) only mentioned
>      positive numbers.  Fifteen years ago, I “fixed” this by changing the
>      type from signed to unsigned.  However, it would still have accepted
>      an input with a leading minus sign (though it would have returned its
>      absolute value).  Fifteen years on, change the type back to signed and
>      fix the logic so it correctly handles both positive and negative
>      numbers without invoking undefined behavior.  This makes it a better
>      match for humanize_number(3), which it is supposed to complement.
>      
>      Fixes:          bbb2703b4f46
>      Reviewed by:    jhb
>      Differential Revision:  https://reviews.freebsd.org/D51542
> ---
>   lib/libutil/expand_number.3 | 56 +++++++++++++++++++------------
>   lib/libutil/expand_number.c | 82 ++++++++++++++++++++++++++++++++++-----------
>   lib/libutil/libutil.h       |  2 +-
>   usr.sbin/ctld/conf.cc       | 12 ++++---
>   usr.sbin/ctld/parse.y       | 26 +++++++-------
>   5 files changed, 118 insertions(+), 60 deletions(-)
> 
> diff --git a/usr.sbin/ctld/conf.cc b/usr.sbin/ctld/conf.cc
> index e86b44ee5004..f3285ebf9d56 100644
> --- a/usr.sbin/ctld/conf.cc
> +++ b/usr.sbin/ctld/conf.cc
> @@ -409,7 +409,8 @@ lun_set_blocksize(size_t value)
>   bool
>   lun_set_device_type(const char *value)
>   {
> -	uint64_t device_type;
> +	const char *errstr;
> +	int device_type;
>   
>   	if (strcasecmp(value, "disk") == 0 ||
>   	    strcasecmp(value, "direct") == 0)
> @@ -421,9 +422,12 @@ lun_set_device_type(const char *value)
>   	    strcasecmp(value, "dvd") == 0 ||
>   	    strcasecmp(value, "dvdrom") == 0)
>   		device_type = T_CDROM;
> -	else if (expand_number(value, &device_type) != 0 || device_type > 15) {
> -		log_warnx("invalid device-type \"%s\" for lun \"%s\"", value,
> -		    lun->l_name);
> +	else {
> +		device_type = strtonum(value, 0, 15, &errstr);
> +		if (errstr != NULL) {
> +			log_warnx("invalid device-type \"%s\" for lun \"%s\"", value,
> +			    lun->l_name);
> +		}
>   		return (false);

This breaks any integer device types.  The return (false) should only be used in
the error case.  I will fix as part of my other ctld changes.

-- 
John Baldwin