Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2015, 07:32 UTC
Message-ID: <xmqqk2ydjvcd.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150319052620.GA30645@peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

> I wonder how much of the boilerplate in the parse_* functions could be
> factored out to use a uintmax_t, with the caller just providing the
> range. That would make it easier to add new types like off_t, and
> possibly even constrained types (e.g., an integer from 0 to 100). On the
> other hand, you mentioned to me elsewhere that there may be some bugs in
> the range-checks of config.c's integer parsing. I suspect they are
> related to exactly this kind of refactoring, so perhaps writing things
> out is best.

I like this idea very well. I wonder if we can implement the family of

    parse_{type}(const char *, unsigned int flags,
    		 const char **endptr, {type} *result)

functions by calls a helper that internally deals with the numbers in uintmax_t, and then checks if the value fits within the possible range of the *result before returning.

    int parse_i(const char *str, unsigned flags,
		const char **endptr, int *result) {
	uintmax_t val;
        int sign = 1, status;
        if (*str == '-') {
		sign = -1; 
                str++;
	}
        status = parse_num(str, flags, endptr, &val, INT_MAX);
        if (status < 0)
        	return status;
	*result = sign < 0 ? -val : val;
        return 0;
    }

(assuming the last parameter to parse_num is used to check if the result fits within that range). Or that may be easier and cleaner to be done in the caller with or something like that:

	status = parse_num(str, flags, endptr, &val);
        if (status < 0)
        	return status;
	if (INT_MAX <= val * sign || val * sign <= INT_MIN) {
        	errno = ERANGE;
                return -1;
	}

If we wanted to, we may even be able to avoid duplication of boilerplate by wrapping the above pattern within a macro, parameterizing the TYPE_{MIN,MAX} and using token pasting, to expand to four necessary result types.

There is no reason for the implementation of the parse_num() helper to be using strtoul(3) or strtoull(3); its behaviour will be under our total control. It can become fancier by enriching the flags bits (e.g. allow scaling factor, etc.) only once and all variants for various result types will get the same feature.