Re: [PATCH v13 02/12] iio: kstrtox: add local _parse_integer_limit_init() helper

[Rodrigo Alencar]

On 26/05/17 10:13AM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> Add parsing helper that accepts an initial value for the accumulated
> result when parsing an 64-bit integer. It reuses current implementation
> for _parse_integer_limit(), which now consumes the new function with
> init = 0. The diff algorithm would have the documentation header and
> prototype of _parse_integer_limit() moved around so it is adjusted
> according to guidelines.

Sashiko's feedback at:
https://sashiko.dev/#/patchset/20260517-adf41513-iio-driver-v13-0-bb6e134a360f%40analog.com?part=2

...

> -/*
> - * Convert non-negative integer string representation in explicitly given radix
> - * to an integer. A maximum of max_chars characters will be converted.
> - *
> - * Return number of characters consumed maybe or-ed with overflow bit.
> - * If overflow occurs, result integer (incorrect) is still returned.
> - *
> - * Don't you dare use this function.
> - */
> -noinline
> -unsigned int _parse_integer_limit(const char *s, unsigned int base, unsigned long long *p,
> -				  size_t max_chars)

...

> +/**
> + * _parse_integer_limit() - Convert integer string representation to an integer
> + *			    limiting the number of characters parsed.
> + * @s: The start of the string.
> + * @base: The number base to use.
> + * @p: Where to write the result of the conversion.
> + * @max_chars: Maximum amount of characters to consume.
> + *
> + * Convert non-negative integer string representation in explicitly given radix
> + * to an integer. A maximum of max_chars characters will be converted.
> + *
> + * Return: Number of characters consumed maybe or-ed with overflow bit.
> + *	   If overflow occurs, result integer (incorrect) is still returned.
> + */
> +noinline
> +unsigned int _parse_integer_limit(const char *s, unsigned int base,
> +				  unsigned long long *p, size_t max_chars)

	Is it safe to remove the "Don't you dare use this function." warning?
	Since this function returns the number of consumed characters bitwise-ORed with
	the KSTRTOX_OVERFLOW flag, turning the comment into standard kernel-doc format
	makes it look like an inviting API.
	If a developer uses this and directly advances a pointer by the return value
	without masking the overflow bit, could an integer overflow during parsing cause
	the pointer to jump out-of-bounds?

That return value merged with the overflow indication is a bit weird in the first
place. I wonder if we need a precursor patch cleaning _parse_integer_limit() and
removing this KSTRTOX_OVERFLOW completely... turning it into:

	ssize_t _parse_integer_limit(const char *s,
				     const char **endp,
				     unsigned int base,
				     unsigned long long *res,
				     size_t max_chars);

so ssize_t returns the amount of converted characters, or -ERANGE if overflow occurs.
Also endp would point to the end of the conversion, so current usage (in _kstrtoull())

	rv = _parse_integer(s, base, &_res);
	if (rv & KSTRTOX_OVERFLOW)
		return -ERANGE;
	if (rv == 0)
		return -EINVAL;
	s += rv;

would become:

	ret = _parse_integer(s, &s, base, &_res);
	if (ret < 0)
		return ret;
	if (!ret)
		return -EINVAL;

which seems more aligned to the code we see out there.

-- 
Kind regards,

Rodrigo Alencar