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@xxxxxx
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@xxxxxx
Thanks!
Patrick
[1]: <89257ab82cd60d135cce02d51eacee7ec35c1c37.camel@xxxxxxxxxxxxxxxxxxx>
[2]: <Z8HW6petWuMRWSXf@xxxxxxxxxxxxxxx>
---
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
