[v2,0/5] parse-options: harden handling of integer values

Message

Patrick Steinhardt April 15, 2025, 12:14 p.m. UTC

Hi,

this patch series addresses the issues raised in [1] and [2]. As
discussed in [1], the series also introduces a couple of safeguards to
make it harder to misuse `OPT_INTEGER()` and `OPT_MAGNITUDE()`:

  - We now track the precision of the underlying integer types. This
    makes it possible to pass arbitrarily-sized integers to those
    options, not only `int` and `unsigned long`, respectively.

  - We introduce a build assert to verify that the passed variable has
    correct signedness.

Furthermore, the series introduces `OPT_UNSIGNED()` to adapt all
callsites that previously used variables with the wrong signedness.

Changes in v2:
  - Adapt computation of upper bounds to use similar logic to
    `maximum_signed_value_of_type()`.
  - Link to v1: https://lore.kernel.org/r/[e-mail]

Thanks!

Patrick

[1]: <[e-mail]>
[2]: <[e-mail]>

---
Patrick Steinhardt (5):
      global: use designated initializers for options
      parse-options: introduce precision handling for `OPTION_INTEGER`
      parse-options: introduce precision handling for `OPTION_MAGNITUDE`
      parse-options: introduce `OPTION_UNSIGNED`
      parse-options: detect mismatches in integer signedness

 apply.c                       |   4 +-
 archive.c                     |  35 ++++++++---
 builtin/am.c                  |  28 ++++++---
 builtin/backfill.c            |   4 +-
 builtin/clone.c               |  13 ++++-
 builtin/column.c              |   2 +-
 builtin/commit-tree.c         |  12 +++-
 builtin/commit.c              |  62 +++++++++++++++-----
 builtin/config.c              |  13 ++++-
 builtin/describe.c            |  24 ++++++--
 builtin/fetch.c               |  10 +++-
 builtin/fmt-merge-msg.c       |  27 ++++++---
 builtin/gc.c                  |  12 +++-
 builtin/grep.c                |  18 ++++--
 builtin/init-db.c             |  13 +++--
 builtin/ls-remote.c           |  11 +++-
 builtin/merge.c               |  38 +++++++++---
 builtin/read-tree.c           |  11 +++-
 builtin/rebase.c              |  25 ++++++--
 builtin/revert.c              |  12 +++-
 builtin/show-branch.c         |  13 ++++-
 builtin/tag.c                 |  24 ++++++--
 builtin/update-index.c        | 131 +++++++++++++++++++++++++++++-------------
 builtin/write-tree.c          |  12 ++--
 diff.c                        |  13 +++--
 git-compat-util.h             |   7 +++
 parse-options.c               | 131 +++++++++++++++++++++++++++++++++++-------
 parse-options.h               |  23 +++++++-
 ref-filter.h                  |  15 +++--
 t/helper/test-parse-options.c |  46 ++++++++++++---
 t/t0040-parse-options.sh      |  57 +++++++++++++++++-
 31 files changed, 657 insertions(+), 189 deletions(-)

Range-diff versus v1:

1:  632c627b9ba = 1:  92d1cbd1153 global: use designated initializers for options
2:  65c450a4395 ! 2:  8d2400d7470 parse-options: introduce precision handling for `OPTION_INTEGER`
    @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_
      	}
      	case OPTION_INTEGER:
     +	{
    -+		intmax_t upper_bound = (((intmax_t) 1 << (opt->precision * 8 - 1)) - 1);
    ++		intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - CHAR_BIT * opt->precision);
     +		intmax_t lower_bound = -upper_bound - 1;
     +		intmax_t value;
     +
3:  4946ca0f702 ! 3:  a9b0d8c1127 parse-options: introduce precision handling for `OPTION_MAGNITUDE`
    @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_
      	}
      	case OPTION_MAGNITUDE:
     +	{
    -+		uintmax_t upper_bound = 0;
    ++		uintmax_t upper_bound = UINTMAX_MAX >> (bitsizeof(uintmax_t) - CHAR_BIT * opt->precision);
     +		unsigned long value;
    -+
    -+		/*
    -+		 * It's stupid, but the obvious way of calculating the upper
    -+		 * bound via `2 ^ n - 1` overflows.
    -+		 */
    -+		for (size_t i = 0; i < opt->precision * 8; i++)
    -+			upper_bound |= ((uintmax_t) 1 << i);
     +
      		if (unset) {
     -			*(unsigned long *)opt->value = 0;
4:  9728d57d5df ! 4:  743afbf539e parse-options: introduce `OPTION_UNSIGNED`
    @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_
      	}
     +	case OPTION_UNSIGNED:
     +	{
    -+		uintmax_t upper_bound = 0;
    ++		uintmax_t upper_bound = UINTMAX_MAX >> (bitsizeof(uintmax_t) - CHAR_BIT * opt->precision);
     +		uintmax_t value;
     +
    -+		/*
    -+		 * It's stupid, but the obvious way of calculating the upper
    -+		 * bound via `2 ^ n - 1` overflows.
    -+		 */
    -+		for (size_t i = 0; i < opt->precision * 8; i++)
    -+			upper_bound |= ((uintmax_t) 1 << i);
    -+
     +		if (unset) {
     +			value = 0;
     +		} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
    @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_
     +	}
      	case OPTION_MAGNITUDE:
      	{
    - 		uintmax_t upper_bound = 0;
    + 		uintmax_t upper_bound = UINTMAX_MAX >> (bitsizeof(uintmax_t) - CHAR_BIT * opt->precision);
     
      ## parse-options.h ##
     @@ parse-options.h: enum parse_opt_type {
5:  59aacc72548 = 5:  1e42672a439 parse-options: detect mismatches in integer signedness

---
base-commit: 5b97a56fa0e7d580dc8865b73107407c9b3f0eff
change-id: 20250401-b4-pks-parse-options-integers-9b4bbcf21011