[PATCH v3 0/7] parse-options: harden handling of integer values
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Apr 16, 2025, 10:02 UTC
- Message-ID
- <20250416-b4-pks-parse-options-integers-v3-0-d390746bea79@pks.im>
- In-Reply-To
- <20250401-b4-pks-parse-options-integers-v1-0-a628ad40c3b4@pks.im>
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.imChanges 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.imThanks!
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 optionsapply.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