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

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

Phillip Wood <phillip.wood123@gmail.com> writes:

>> There are a few choices regarding the interface and the implementation.
>> 
>> First, the implementation:
>> ...
> This all sounds sensible to me and an does the interface description.
> ...
> If we're parsing INTMAX_MIN then this negation tries to calculate 
> -INTMAX_MIN which is undefined (I've added some tests for parsing 
> INTMAX_MAX and INTMAX_MIN at [1] and verified that UBSAN is triggered 
> when parsing INTMAX_MIN). We could do
>
> 		*ret = u_ret;
> 		if (*ret != INTMAX_MIN)
> 			*ret = -*ret;
>
> but I think it might be easier to alter parse_from_buf_internal() to 
> make "negate" a local variable, change the function argument to "bool 
> allow_negative" and do
>
> 		*ret = negate ? 0u - val : val;
>
> Then parse_signed_from_buf() can do "*ret = *u_ret;" to convert the 
> output of parse_from_buf_internal() to a signed value.
>
>> 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;
>
> Do we want to set errno=0 here so that we can be sure it has been set by 
> parse_int_from_buf() when we check it below?


After this message, the discussion stopped and the topic has been
dormant since then for a month and a half.  I'd drop the topic from
'seen' soonish but that does not mean an improved version of this
patch is unwelcome.

Thanks.