mbox series

[v3,0/7] parse-options: harden handling of integer values

Message ID 20250416-b4-pks-parse-options-integers-v3-0-d390746bea79@pks.im (mailing list archive)
Headers show
Series parse-options: harden handling of integer values | expand

Message

Patrick Steinhardt April 16, 2025, 10:02 a.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/20250401-b4-pks-parse-options-integers-v1-0-a628ad40c3b4@pks.im

Changes in v3:
  - Introduce `errno` checks for `strto{u,i}max()`.
  - Note that the precision is in bytes.
  - Reject leading '-' when parsing unsigned integers.
  - Introduce bounded integer options. This patch is mostly a proof of
    concept that demonstrates that precision and ranges are orthogonal
    to one another, so I consider it to be an optional patch. It may be
    useful in the future, but I haven't converted any callsites to use
    bounds yet.
  - Link to v2: https://lore.kernel.org/r/20250415-b4-pks-parse-options-integers-v2-0-ce07441a1f01@pks.im

Thanks!

Patrick

[1]: <89257ab82cd60d135cce02d51eacee7ec35c1c37.camel@physik.fu-berlin.de>
[2]: <Z8HW6petWuMRWSXf@teonanacatl.net>

---
Patrick Steinhardt (7):
      global: use designated initializers for options
      parse-options: check for overflow when parsing integers
      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
      parse-options: introduce bounded integer options

 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               | 176 +++++++++++++++++++++++++++++++++++++-----
 parse-options.h               |  75 +++++++++++++++++-
 ref-filter.h                  |  15 ++--
 t/helper/test-parse-options.c |  51 ++++++++++--
 t/t0040-parse-options.sh      | 102 +++++++++++++++++++++++-
 31 files changed, 805 insertions(+), 188 deletions(-)

Range-diff versus v2:

1:  80c8b943e9b = 1:  3dc362ae91b global: use designated initializers for options
-:  ----------- > 2:  a5a71ad6da8 parse-options: check for overflow when parsing integers
2:  307eaeb463c ! 3:  1ad133c639e parse-options: introduce precision handling for `OPTION_INTEGER`
    @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_
     +		} else if (!*arg) {
      			return error(_("%s expects a numerical value"),
      				     optname(opt, flags));
    --		*(int *)opt->value = strtol(arg, (char **)&s, 10);
    --		if (*s)
    --			return error(_("%s expects a numerical value"),
    --				     optname(opt, flags));
    --		return 0;
     +		} else {
    ++			errno = 0;
     +			value = strtoimax(arg, (char **)&s, 10);
     +			if (*s)
     +				return error(_("%s expects a numerical value"),
     +					     optname(opt, flags));
    -+
    ++			if (errno == ERANGE)
    ++				return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
    ++					     arg, optname(opt, flags), lower_bound, upper_bound);
    ++			if (errno)
    ++				return error_errno(_("value %s for %s cannot be parsed"),
    ++						   arg, optname(opt, flags));
     +		}
      
    +-		errno = 0;
    +-		*(int *)opt->value = strtol(arg, (char **)&s, 10);
    +-		if (*s)
    +-			return error(_("%s expects a numerical value"),
    +-				     optname(opt, flags));
    +-		if (errno == ERANGE)
     +		if (value < lower_bound || value > upper_bound)
    -+			return error(_("value %"PRIdMAX" for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
    -+				     value, optname(opt, flags), lower_bound, upper_bound);
    -+
    + 			return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
    +-				     arg, optname(opt, flags), (intmax_t)LONG_MIN, (intmax_t)LONG_MAX);
    +-		if (errno)
    +-			return error_errno(_("value %s for %s cannot be parsed"),
    +-					   arg, optname(opt, flags));
    ++				     arg, optname(opt, flags), lower_bound, upper_bound);
    + 
    +-		return 0;
     +		switch (opt->precision) {
     +		case 1:
     +			*(int8_t *)opt->value = value;
    @@ parse-options.h: typedef int parse_opt_subcommand_fn(int argc, const char **argv
       *   stores pointers to the values to be filled.
       *
     + * `precision`::
    -+ *   precision of the integer pointed to by `value`. Should typically be its
    -+ *   `sizeof()`.
    ++ *   precision of the integer pointed to by `value` in number of bytes. Should
    ++ *   typically be its `sizeof()`.
     + *
       * `argh`::
       *   token to explain the kind of argument this option wants. Does not
    @@ t/t0040-parse-options.sh: test_expect_success 'OPT_NUMBER_CALLBACK() works' '
      magnitude: 0
      timestamp: 0
      string: (not set)
    -@@ t/t0040-parse-options.sh: test_expect_success 'magnitude with units but no numbers' '
    +@@ t/t0040-parse-options.sh: test_expect_success 'overflowing integer' '
      	test_must_be_empty out
      '
      
3:  5bfb9af2262 ! 4:  01e56b43c77 parse-options: introduce precision handling for `OPTION_MAGNITUDE`
    @@ Commit message
     
      ## parse-options.c ##
     @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
    + 
    + 		if (value < lower_bound || value > upper_bound)
    + 			return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
    +-				     arg, optname(opt, flags), lower_bound, upper_bound);
    ++				     arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);
    + 
    + 		switch (opt->precision) {
    + 		case 1:
    +@@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
      		}
      	}
      	case OPTION_MAGNITUDE:
    @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_
     +		}
     +
     +		if (value > upper_bound)
    -+			return error(_("value %"PRIuMAX" for %s exceeds %"PRIuMAX),
    -+				     (uintmax_t) value, optname(opt, flags), upper_bound);
    ++			return error(_("value %s for %s not in range [%"PRIuMAX",%"PRIuMAX"]"),
    ++				     arg, optname(opt, flags), (uintmax_t)0, (uintmax_t)upper_bound);
     +
     +		switch (opt->precision) {
     +		case 1:
    @@ t/t0040-parse-options.sh: test_expect_success 'i16 limits range' '
     +	test-tool parse-options --m16 65535 >out &&
     +	test_grep "m16: 65535" out &&
     +	test_must_fail test-tool parse-options --m16 65536 2>err &&
    -+	test_grep "value 65536 for option .m16. exceeds 65535" err
    ++	test_grep "value 65536 for option .m16. not in range \[0,65535\]" err
     +'
     +
      test_done
4:  75ff495b897 ! 5:  e7458eadea9 parse-options: introduce `OPTION_UNSIGNED`
    @@ Commit message
         parse-options: introduce `OPTION_UNSIGNED`
     
         We have two generic ways to parse integers in the "parse-options"
    -    subsytem:
    +    subsystem:
     
           - `OPTION_INTEGER` parses a signed integer.
     
    @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_
     +		} else if (!*arg) {
     +			return error(_("%s expects a numerical value"),
     +				     optname(opt, flags));
    ++		} else if (*arg == '-') {
    ++			return error(_("%s does not accept negative values"),
    ++				     optname(opt, flags));
     +		} else {
    ++			errno = 0;
     +			value = strtoumax(arg, (char **)&s, 10);
     +			if (*s)
     +				return error(_("%s expects a numerical value"),
     +					     optname(opt, flags));
    ++			if (errno == ERANGE)
    ++				return error(_("value %s for %s not in range [%"PRIuMAX",%"PRIuMAX"]"),
    ++					     arg, optname(opt, flags), (uintmax_t)0, (uintmax_t)upper_bound);
    ++			if (errno)
    ++				return error_errno(_("value %s for %s cannot be parsed"),
    ++						   arg, optname(opt, flags));
    ++
     +		}
     +
     +		if (value > upper_bound)
    -+			return error(_("value %"PRIuMAX" for %s exceeds %"PRIuMAX),
    -+				     value, optname(opt, flags), upper_bound);
    ++			return error(_("value %s for %s not in range [%"PRIuMAX",%"PRIuMAX"]"),
    ++				     arg, optname(opt, flags), (uintmax_t)0, (uintmax_t)upper_bound);
     +
     +		switch (opt->precision) {
     +		case 1:
    -+			*(int8_t *)opt->value = value;
    ++			*(uint8_t *)opt->value = value;
     +			return 0;
     +		case 2:
    -+			*(int16_t *)opt->value = value;
    ++			*(uint16_t *)opt->value = value;
     +			return 0;
     +		case 4:
    -+			*(int32_t *)opt->value = value;
    ++			*(uint32_t *)opt->value = value;
     +			return 0;
     +		case 8:
    -+			*(int64_t *)opt->value = value;
    ++			*(uint64_t *)opt->value = value;
     +			return 0;
     +		default:
     +			BUG("invalid precision for option %s",
    @@ t/t0040-parse-options.sh: cat >expect <<\EOF
      m16: 0
      timestamp: 0
     @@ t/t0040-parse-options.sh: test_expect_success 'm16 limits range' '
    - 	test_grep "value 65536 for option .m16. exceeds 65535" err
    + 	test_grep "value 65536 for option .m16. not in range \[0,65535\]" err
      '
      
     +test_expect_success 'u16 limits range' '
     +	test-tool parse-options --u16 65535 >out &&
     +	test_grep "u16: 65535" out &&
     +	test_must_fail test-tool parse-options --u16 65536 2>err &&
    -+	test_grep "value 65536 for option .u16. exceeds 65535" err
    ++	test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
    ++'
    ++
    ++test_expect_success 'u16 does not accept negative value' '
    ++	test_must_fail test-tool parse-options --u16 -1 >out 2>err &&
    ++	test_grep "option .u16. does not accept negative values" err &&
    ++	test_must_be_empty out
     +'
     +
      test_done
5:  20ab753baef = 6:  5b05451c346 parse-options: detect mismatches in integer signedness
-:  ----------- > 7:  7115a7a31aa parse-options: introduce bounded integer options

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