Re: [PATCH v3 3/7] parse-options: introduce precision handling for `OPTION_INTEGER`

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

Patrick Steinhardt <ps@pks.im> writes:

> The `OPTION_INTEGER` option type accepts a signed integer. The type of
> the underlying integer is a simple `int`, which restricts the range of
> values accepted by such options. But there is a catch: because the
> caller provides a pointer to the value via the `.value` field, which is
> a simple void pointer. This has two consequences:
>
>   - There is no check whether the passed value is sufficiently long to
>     store the entire range of `int`. This can lead to integer wraparound
>     in the best case and out-of-bounds writes in the worst case.
>
>   - Even when a caller knows that they want to store a value larger than
>     `INT_MAX` they don't have a way to do so.
>
> Funny enough, even if the caller gets everything correct the parsing
> logic is still insufficient because we use `strtol()` to parse the
> argument, which returns a `long`. But as that value is implicitly cast
> when assigning it to the `int` field we may still get invalid results.
>
> In practice this doesn't tend to be a huge issue because users typically
> don't end up passing huge values to most commands. But the parsing logic
> is demonstrably broken, and it is too easy to get the calling convention
> wrong.
>
> Improve the situation by introducing a new `precision` field into the
> structure. This field gets assigned automatically by `OPT_INTEGER_F()`
> and tracks the size of the passed value. Like this it becomes possible
> for the caller to pass arbitrarily-sized integers and the underlying
> logic knows to handle it correctly by doing range checks. Furthermore,
> convert the code to use `strtoimax()` intstead of `strtol()` so that we
> can also parse values larger than `LONG_MAX`.
>
> Note that we do not yet assert signedness of the passed variable, which
> is another source of bugs. This will be handled in a subsequent commit.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ...
> +	{
> +		intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - CHAR_BIT * opt->precision);
> +		intmax_t lower_bound = -upper_bound - 1;
> +		intmax_t value;
> +
>  		if (unset) {
> ...
> +		if (value < lower_bound || value > upper_bound)
>  			return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),

When I imagined the case where precision is set to the same size as
sizeof(intmax_t), this comparison made my head spin, but I think it
should safely yield false for any "value", in which case it is fine.

> +		switch (opt->precision) {
> +		case 1:
> +			*(int8_t *)opt->value = value;
> +			return 0;
> +		case 2:
> +			*(int16_t *)opt->value = value;
> +			return 0;
> +		case 4:
> +			*(int32_t *)opt->value = value;
> +			return 0;
> +		case 8:
> +			*(int64_t *)opt->value = value;
> +			return 0;
> +		default:

Good to have this "default" arm.  As you cannot take an address of a
bitfield, you cannot pass a bitfield member in a struct that is set
to say 24-bit wide with .precision set to 3.  IOW, limiting ourselves
only to sizes of naturally occurring integral types is sufficient.

> +			BUG("invalid precision for option %s",
> +			    optname(opt, flags));
> +		}
> +	}
>  	case OPTION_MAGNITUDE:
>  		if (unset) {
>  			*(unsigned long *)opt->value = 0;
> diff --git a/parse-options.h b/parse-options.h
> index 997ffbee805..8db96402c4d 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -92,6 +92,10 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
>   * `value`::
>   *   stores pointers to the values to be filled.
>   *
> + * `precision`::
> + *   precision of the integer pointed to by `value` in number of bytes. Should
> + *   typically be its `sizeof()`.
> + *

Nicer to see "bytes" in the description.  I am still puzzled, and
more importantly, I suspect readers will be puzzled, when told
"Should typically be", as it implies there may be valid cases when
it can be different from it, and wanting to know what these cases
are.

> @@ -214,6 +219,7 @@ struct option {
>  	.short_name = (s), \
>  	.long_name = (l), \
>  	.value = (v), \
> +	.precision = sizeof(*v), \
>  	.argh = N_("n"), \
>  	.help = (h), \
>  	.flags = (f), \

Nice.  By doing this in OPT_INTEGER_F(), we cover OPT_INTEGER() as
well automatically.

Thanks.