parse-options: introduce precision handling for `OPTION_INTEGER`

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. 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Apr 17, 2025 at 12:49 UTC 09705696f763bac370ac74926bef137eb712c0c8
8 files changed +75 -14
builtin/fmt-merge-msg.c
+2
@@ -24,6 +24,7 @@ int cmd_fmt_merge_msg(int argc,
24 .type = OPTION_INTEGER,
25 .long_name = "log",
26 .value = &shortlog_len,
27 + .precision = sizeof(shortlog_len),
28 .argh = N_("n"),
29 .help = N_("populate log with at most <n> entries from shortlog"),
30 .flags = PARSE_OPT_OPTARG,
@@ -33,6 +34,7 @@ int cmd_fmt_merge_msg(int argc,
34 .type = OPTION_INTEGER,
35 .long_name = "summary",
36 .value = &shortlog_len,
37 + .precision = sizeof(shortlog_len),
38 .argh = N_("n"),
39 .help = N_("alias for --log (deprecated)"),
40 .flags = PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN,
builtin/merge.c
+1
@@ -254,6 +254,7 @@ static struct option builtin_merge_options[] = {
254 .type = OPTION_INTEGER,
255 .long_name = "log",
256 .value = &shortlog_len,
257 + .precision = sizeof(shortlog_len),
258 .argh = N_("n"),
259 .help = N_("add (at most <n>) entries from shortlog to merge commit message"),
260 .flags = PARSE_OPT_OPTARG,
builtin/show-branch.c
+1
@@ -671,6 +671,7 @@ int cmd_show_branch(int ac,
671 .type = OPTION_INTEGER,
672 .long_name = "more",
673 .value = &extra,
674 + .precision = sizeof(extra),
675 .argh = N_("n"),
676 .help = N_("show <n> more commits after the common ancestor"),
677 .flags = PARSE_OPT_OPTARG,
builtin/tag.c
+1
@@ -483,6 +483,7 @@ int cmd_tag(int argc,
483 .type = OPTION_INTEGER,
484 .short_name = 'n',
485 .value = &filter.lines,
486 + .precision = sizeof(filter.lines),
487 .argh = N_("n"),
488 .help = N_("print <n> lines of each tag message"),
489 .flags = PARSE_OPT_OPTARG,
parse-options.c
+39 -13
@@ -172,25 +172,51 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
172 return (*opt->ll_callback)(p, opt, p_arg, p_unset);
173 }
174 case OPTION_INTEGER:
175 + {
176 + intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - CHAR_BIT * opt->precision);
177 + intmax_t lower_bound = -upper_bound - 1;
178 + intmax_t value;
179 +
180 if (unset) {
176 - *(int *)opt->value = 0;
177 - return 0;
178 - }
179 - if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
180 - *(int *)opt->value = opt->defval;
181 - return 0;
182 - }
183 - if (get_arg(p, opt, flags, &arg))
181 + value = 0;
182 + } else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
183 + value = opt->defval;
184 + } else if (get_arg(p, opt, flags, &arg)) {
185 return -1;
185 - if (!*arg)
186 + } else if (!*arg) {
187 return error(_("%s expects a numerical value"),
188 optname(opt, flags));
188 - if (!git_parse_int(arg, opt->value))
189 - return error(_("%s expects an integer value"
190 - " with an optional k/m/g suffix"),
189 + } else if (!git_parse_signed(arg, &value, upper_bound)) {
190 + if (errno == ERANGE)
191 + return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
192 + arg, optname(opt, flags), lower_bound, upper_bound);
193 +
194 + return error(_("%s expects an integer value with an optional k/m/g suffix"),
195 optname(opt, flags));
192 - return 0;
196 + }
197 +
198 + if (value < lower_bound)
199 + return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
200 + arg, optname(opt, flags), lower_bound, upper_bound);
201
202 + switch (opt->precision) {
203 + case 1:
204 + *(int8_t *)opt->value = value;
205 + return 0;
206 + case 2:
207 + *(int16_t *)opt->value = value;
208 + return 0;
209 + case 4:
210 + *(int32_t *)opt->value = value;
211 + return 0;
212 + case 8:
213 + *(int64_t *)opt->value = value;
214 + return 0;
215 + default:
216 + BUG("invalid precision for option %s",
217 + optname(opt, flags));
218 + }
219 + }
220 case OPTION_UNSIGNED:
221 if (unset) {
222 *(unsigned long *)opt->value = 0;
parse-options.h
+6
@@ -92,6 +92,10 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
92 * `value`::
93 * stores pointers to the values to be filled.
94 *
95 + * `precision`::
96 + * precision of the integer pointed to by `value` in number of bytes. Should
97 + * typically be its `sizeof()`.
98 + *
99 * `argh`::
100 * token to explain the kind of argument this option wants. Does not
101 * begin in capital letter, and does not end with a full stop.
@@ -151,6 +155,7 @@ struct option {
155 int short_name;
156 const char *long_name;
157 void *value;
158 + size_t precision;
159 const char *argh;
160 const char *help;
161
@@ -214,6 +219,7 @@ struct option {
219 .short_name = (s), \
220 .long_name = (l), \
221 .value = (v), \
222 + .precision = sizeof(*v), \
223 .argh = N_("n"), \
224 .help = (h), \
225 .flags = (f), \
t/helper/test-parse-options.c
+3
@@ -120,6 +120,7 @@ int cmd__parse_options(int argc, const char **argv)
120 };
121 struct string_list expect = STRING_LIST_INIT_NODUP;
122 struct string_list list = STRING_LIST_INIT_NODUP;
123 + int16_t i16 = 0;
124
125 struct option options[] = {
126 OPT_BOOL(0, "yes", &boolean, "get a boolean"),
@@ -139,6 +140,7 @@ int cmd__parse_options(int argc, const char **argv)
140 OPT_NEGBIT(0, "neg-or4", &boolean, "same as --no-or4", 4),
141 OPT_GROUP(""),
142 OPT_INTEGER('i', "integer", &integer, "get a integer"),
143 + OPT_INTEGER(0, "i16", &i16, "get a 16 bit integer"),
144 OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
145 OPT_UNSIGNED('u', "unsigned", &unsigned_integer, "get an unsigned integer"),
146 OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
@@ -210,6 +212,7 @@ int cmd__parse_options(int argc, const char **argv)
212 }
213 show(&expect, &ret, "boolean: %d", boolean);
214 show(&expect, &ret, "integer: %d", integer);
215 + show(&expect, &ret, "i16: %"PRIdMAX, (intmax_t) i16);
216 show(&expect, &ret, "unsigned: %lu", unsigned_integer);
217 show(&expect, &ret, "timestamp: %"PRItime, timestamp);
218 show(&expect, &ret, "string: %s", string ? string : "(not set)");
t/t0040-parse-options.sh
+22 -1
@@ -22,6 +22,7 @@ usage: test-tool parse-options <options>
22
23 -i, --[no-]integer <n>
24 get a integer
25 + --[no-]i16 <n> get a 16 bit integer
26 -j <n> get a integer, too
27 -u, --unsigned <n> get an unsigned integer
28 --[no-]set23 set integer to 23
@@ -138,6 +139,7 @@ test_expect_success 'OPT_UNSIGNED() 3giga' '
139 cat >expect <<\EOF
140 boolean: 2
141 integer: 1729
142 +i16: 0
143 unsigned: 16384
144 timestamp: 0
145 string: 123
@@ -158,6 +160,7 @@ test_expect_success 'short options' '
160 cat >expect <<\EOF
161 boolean: 2
162 integer: 1729
163 +i16: 9000
164 unsigned: 16384
165 timestamp: 0
166 string: 321
@@ -169,7 +172,7 @@ file: prefix/fi.le
172 EOF
173
174 test_expect_success 'long options' '
172 - test-tool parse-options --boolean --integer 1729 --unsigned 16k \
175 + test-tool parse-options --boolean --integer 1729 --i16 9000 --unsigned 16k \
176 --boolean --string2=321 --verbose --verbose --no-dry-run \
177 --abbrev=10 --file fi.le --obsolete \
178 >output 2>output.err &&
@@ -181,6 +184,7 @@ test_expect_success 'abbreviate to something longer than SHA1 length' '
184 cat >expect <<-EOF &&
185 boolean: 0
186 integer: 0
187 + i16: 0
188 unsigned: 0
189 timestamp: 0
190 string: (not set)
@@ -255,6 +259,7 @@ test_expect_success 'superfluous value provided: cmdmode' '
259 cat >expect <<\EOF
260 boolean: 1
261 integer: 13
262 +i16: 0
263 unsigned: 0
264 timestamp: 0
265 string: 123
@@ -278,6 +283,7 @@ test_expect_success 'intermingled arguments' '
283 cat >expect <<\EOF
284 boolean: 0
285 integer: 2
286 +i16: 0
287 unsigned: 0
288 timestamp: 0
289 string: (not set)
@@ -345,6 +351,7 @@ cat >expect <<\EOF
351 Callback: "four", 0
352 boolean: 5
353 integer: 4
354 +i16: 0
355 unsigned: 0
356 timestamp: 0
357 string: (not set)
@@ -370,6 +377,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
377 cat >expect <<\EOF
378 boolean: 1
379 integer: 23
380 +i16: 0
381 unsigned: 0
382 timestamp: 0
383 string: (not set)
@@ -449,6 +457,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
457 cat >expect <<\EOF
458 boolean: 0
459 integer: 0
460 +i16: 0
461 unsigned: 0
462 timestamp: 0
463 string: (not set)
@@ -785,4 +794,16 @@ test_expect_success 'unsigned with units but no numbers' '
794 test_must_be_empty out
795 '
796
797 +test_expect_success 'i16 limits range' '
798 + test-tool parse-options --i16 32767 >out &&
799 + test_grep "i16: 32767" out &&
800 + test_must_fail test-tool parse-options --i16 32768 2>err &&
801 + test_grep "value 32768 for option .i16. not in range \[-32768,32767\]" err &&
802 +
803 + test-tool parse-options --i16 -32768 >out &&
804 + test_grep "i16: -32768" out &&
805 + test_must_fail test-tool parse-options --i16 -32769 2>err &&
806 + test_grep "value -32769 for option .i16. not in range \[-32768,32767\]" err
807 +'
808 +
809 test_done