Re: [PATCH GSoC v15 02/13] git-compat-util: add `strtoumax_szt()` with error handling

[Junio C Hamano <gitster@pobox.com>]

Pablo Sabater <pabloosabaterr@gmail.com> writes:

> From: Eric Ju <eric.peijian@gmail.com>
>
> We already have `strtoul_ui()` and similar functions that provide proper
> error handling using `strtoul` from the standard library. However,
> there isn't currently a variant that returns a `size_t`.
>
> Using `strtoul` is unreliable because `size_`t is platform-dependent,
> `unsigned long` could be too big to fit into a `size_t` or too small to
> hold a `size_t`.

It is somehow annoying to see that the commit log desciption, which
is *clearly* meant to be plaintext, is so heavily riddled with
backquoted references to code/program symbols.  Yes, `literal` is a
correct way to format them in both AsciiDoc and Markdown, so we very
much welcome them in our documentation, but not in proposed log
messages.

In any case, you dropped 't' in 'size_t' outside the pair of
backquotes.

> Use `strtoumax` which returns a `uintmax_t` guaranteed to be at least as

`strtoumax()`, as the convention you used above for strtoul_ui() is
to suffix function names with ().

> large as `size_t`, add a range check against `SIZE_MAX` to prevent
> `size_t` overflow.

OK.

> This variant is needed in a subsequent commit to enable returning a
> `size_t` with proper error handling.
>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  git-compat-util.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 8809776407..5ecce5bbd2 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -975,6 +975,26 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
>  	return 0;
>  }
>  
> +/*
> + * Convert a string to a size_t using the standard library's strtoumax, with
> + * additional error handling to ensure robustness.
> + */
> +static inline int strtoumax_szt(char const *s, int base, size_t *result)
> +{
> +	uintmax_t uim;
> +	char *p;
> +
> +	errno = 0;
> +	/* negative values would be accepted by strtoul */
> +	if (strchr(s, '-'))
> +		return -1;

Hmph, wouldn't 

	if (*s == '-' || !*s)
		return -1

cut it?  Since your call to strtoumax() checks that the string was
parsed to the end by insisting *p is NUL?

If you are trying to more explicitly insist that s[] has only
digits, which may not be a bad idea, as that is what we generally
expect, then

	if (!s[0] || s[strspn(s, "0123456789")])
		return -1;

perhaps.

> +	uim = strtoumax(s, &p, base);
> +	if ((errno || *p || p == s) || uim > SIZE_MAX)
> +		return -1;

And with !s[0] upfront, we can discard (p==s) case from here.  Other
strto*() wrappers we have may need the "cannot be empty" check,
because they do not need any upfront validation of s[] like we do
here (we do so to reject negative numbers), but since we do need to
check s[] before calling the system strto*() function anyway, it is
OK to be different here from the others.

If uintmax_t and size_t are of the same width, then (SIZE_MAX < uim)
becomes mathmatically impossible, but hopefully no compiler or
static checker is stupid enough to warn against it.

> +	*result = uim;
> +	return 0;
> +}
> +
>  static inline int strtol_i(char const *s, int base, int *result)
>  {
>  	long ul;