Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers

> If you have a buffer that is not NUL-terminated but want to parse an
> integer, there aren't many good options. If you use strtol() and
> friends, you risk running off the end of the buffer if there is no
> non-digit terminating character. And even if you carefully make sure
> that there is such a character, ASan's strict-string-check mode will
> still complain.
> 
> You can copy bytes into a temporary buffer, terminate it, and then call
> strtol(), but doing so adds some pitfalls (like making sure you soak up
> whitespace and leading +/- signs, and reporting overflow for overly long
> input). Or you can hand-parse the digits, but then you need to take some
> care to handle overflow (and again, whitespace and +/- signs).
> 
> These things aren't impossible to do right, but it's error-prone to have
> to do them in every spot that wants to do such parsing. So let's add
> some functions which can be used across the code base.
> 
> There are a few choices regarding the interface and the implementation.
> 
> First, the implementation:
> 
>    - I went with with parsing the digits (rather than buffering and
>      passing to libc functions). It ends up being a similar amount of
>      code because we have to do some parsing either way. And likewise
>      overflow detection depends on the exact type the caller wants, so we
>      either have to do it by hand or write a separate wrapper for
>      strtol(), strtoumax(), and so on.
> 
>    - Unsigned overflow detection is done using the same techniques as in
>      unsigned_add_overflows(), etc. We can't use those macros directly
>      because our core function is type-agnostic (so the caller passes in
>      the max value, rather than us deriving it on the fly). This is
>      similar to how git_parse_int(), etc, work.
> 
>    - Signed overflow detection assumes that we can express a negative
>      value with magnitude one larger than our maximum positive value
>      (e.g., -128..127 for a signed 8-bit value). I doubt this is
>      guaranteed by the standard, but it should hold in practice, and we
>      make the same assumption in git_parse_int(), etc. The nice thing
>      about this is that we can derive the range from the number of bits
>      in the type. For ints, you obviously could use INT_MIN..INT_MAX, but
>      for an arbitrary type, we can use maximum_signed_value_of_type().
> 
>    - I didn't bother with handling bases other than 10. It would
>      complicate the code, and I suspect it won't be needed. We could
>      probably retro-fit it later without too much work, if need be.

> +bool parse_unsigned_from_buf(const char *buf, size_t len, const char **ep,
> +			     uintmax_t *ret, uintmax_t max)
> +{
> +	return parse_from_buf_internal(buf, len, ep, NULL, ret, max);
> +}
> +
> +bool parse_signed_from_buf(const char *buf, size_t len, const char **ep,
> +			   intmax_t *ret, intmax_t max)
> +{
> +	uintmax_t u_ret;
> +	bool negate;
> +
> +	if (!parse_from_buf_internal(buf, len, ep, &negate, &u_ret, max))
> +		return false;
> +	/*
> +	 * Range already checked internally, but we must apply negation
> +	 * ourselves since only we have the signed integer type.
> +	 */
> +	if (negate) {
> +		*ret = u_ret;
> +		*ret = -*ret;

> diff --git a/t/unit-tests/u-parse-int.c b/t/unit-tests/u-parse-int.c
> new file mode 100644
> index 0000000000..a1601bb16b
> --- /dev/null
> +++ b/t/unit-tests/u-parse-int.c
> @@ -0,0 +1,98 @@
> +#include "unit-test.h"
> +#include "parse.h"
> +
> +static void check_int(const char *buf, size_t len,
> +		      size_t expect_ep_ofs, int expect_errno,
> +		      int expect_result)
> +{
> +	const char *ep;
> +	int result;

> +	bool ok = parse_int_from_buf(buf, len, &ep, &result);
> +
> +	if (expect_errno) {
> +		cl_assert(!ok);
> +		cl_assert_equal_i(expect_errno, errno);
> +		return;
> +	}
> +
> +	cl_assert(ok);
> +	cl_assert_equal_i(expect_result, result);
> +	cl_assert_equal_i(expect_ep_ofs, ep - buf);
> +}
> +
> +static void check_int_str(const char *buf, size_t ofs, int err, int res)
> +{
> +	check_int(buf, strlen(buf), ofs, err, res);
> +}
> +
> +static void check_int_full(const char *buf, int res)
> +{
> +	check_int_str(buf, strlen(buf), 0, res);
> +}
> +
> +static void check_int_err(const char *buf, int err)
> +{
> +	check_int(buf, strlen(buf), 0, err, 0);
> +}
> +
> +void test_parse_int__basic(void)
> +{
> +	cl_invoke(check_int_full("0", 0));
> +	cl_invoke(check_int_full("11", 11));
> +	cl_invoke(check_int_full("-23", -23));
> +	cl_invoke(check_int_full("+23", 23));
> +
> +	cl_invoke(check_int_str("  31337  ", 7, 0, 31337));
> +
> +	cl_invoke(check_int_err("  garbage", EINVAL));
> +	cl_invoke(check_int_err("", EINVAL));
> +	cl_invoke(check_int_err("-", EINVAL));
> +
> +	cl_invoke(check_int("123", 2, 2, 0, 12));
> +}
> +
> +void test_parse_int__range(void)
> +{
> +	/*
> +	 * These assume a 32-bit int. We could avoid that with some
> +	 * conditionals, but it's probably better for the test to
> +	 * fail noisily and we can decide how to handle it then.
> +	 */
> +	cl_invoke(check_int_full("2147483647", 2147483647));
> +	cl_invoke(check_int_err("2147483648", ERANGE));
> +	cl_invoke(check_int_full("-2147483647", -2147483647));
> +	cl_invoke(check_int_full("-2147483648", -2147483648));
> +	cl_invoke(check_int_err("-2147483649", ERANGE));
> +}
> +
> +static void check_unsigned(const char *buf, uintmax_t max,
> +			   int expect_errno, uintmax_t expect_result)
> +{
> +	const char *ep;
> +	uintmax_t result;
> +	bool ok = parse_unsigned_from_buf(buf, strlen(buf), &ep, &result, max);
> +
> +	if (expect_errno) {
> +		cl_assert(!ok);
> +		cl_assert_equal_i(expect_errno, errno);
> +		return;
> +	}
> +
> +	cl_assert(ok);
> +	cl_assert_equal_s(ep, "");
> +	/*
> +	 * Do not use cl_assert_equal_i_fmt(..., PRIuMAX) here. The macro
> +	 * casts to int under the hood, corrupting the values.
> +	 */
> +	clar__assert_equal(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC,
> +			   CLAR_CURRENT_LINE,
> +			   "expect_result != result", 1,
> +			   "%"PRIuMAX, expect_result, result);
> +}
> +
> +void test_parse_int__unsigned(void)
> +{
> +	cl_invoke(check_unsigned("4294967295", UINT_MAX, 0, 4294967295U));
> +	cl_invoke(check_unsigned("1053", 1000, ERANGE, 0));
> +	cl_invoke(check_unsigned("-17", UINT_MAX, EINVAL, 0));
> +}