Skip to content

Commit 38bba07

Browse files
pks-tgitster
pks-t
authored and
gitster
committed
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. Funny enough, even if the caller gets everything correct the parsing logic is still insufficient because we use `strtol()` to parse the argument, which returns a `long`. But as that value is implicitly cast when assigning it to the `int` field we may still get invalid results. 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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 69f4c95 commit 38bba07

File tree

8 files changed

+79
-21
lines changed

8 files changed

+79
-21
lines changed

‎builtin/fmt-merge-msg.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ int cmd_fmt_merge_msg(int argc,
2424
.type = OPTION_INTEGER,
2525
.long_name = "log",
2626
.value = &shortlog_len,
27+
.precision = sizeof(shortlog_len),
2728
.argh = N_("n"),
2829
.help = N_("populate log with at most <n> entries from shortlog"),
2930
.flags = PARSE_OPT_OPTARG,
@@ -33,6 +34,7 @@ int cmd_fmt_merge_msg(int argc,
3334
.type = OPTION_INTEGER,
3435
.long_name = "summary",
3536
.value = &shortlog_len,
37+
.precision = sizeof(shortlog_len),
3638
.argh = N_("n"),
3739
.help = N_("alias for --log (deprecated)"),
3840
.flags = PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN,

‎builtin/merge.c‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,7 @@ static struct option builtin_merge_options[] = {
254254
.type = OPTION_INTEGER,
255255
.long_name = "log",
256256
.value = &shortlog_len,
257+
.precision = sizeof(shortlog_len),
257258
.argh = N_("n"),
258259
.help = N_("add (at most <n>) entries from shortlog to merge commit message"),
259260
.flags = PARSE_OPT_OPTARG,

‎builtin/show-branch.c‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -671,6 +671,7 @@ int cmd_show_branch(int ac,
671671
.type = OPTION_INTEGER,
672672
.long_name = "more",
673673
.value = &extra,
674+
.precision = sizeof(extra),
674675
.argh = N_("n"),
675676
.help = N_("show <n> more commits after the common ancestor"),
676677
.flags = PARSE_OPT_OPTARG,

‎builtin/tag.c‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,7 @@ int cmd_tag(int argc,
483483
.type = OPTION_INTEGER,
484484
.short_name = 'n',
485485
.value = &filter.lines,
486+
.precision = sizeof(filter.lines),
486487
.argh = N_("n"),
487488
.help = N_("print <n> lines of each tag message"),
488489
.flags = PARSE_OPT_OPTARG,

‎parse-options.c‎

Lines changed: 43 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -172,33 +172,56 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
172172
return (*opt->ll_callback)(p, opt, p_arg, p_unset);
173173
}
174174
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+
175180
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)) {
184185
return -1;
185-
if (!*arg)
186+
} else if (!*arg) {
186187
return error(_("%s expects a numerical value"),
187188
optname(opt, flags));
189+
} else {
190+
errno = 0;
191+
value = strtoimax(arg, (char **)&s, 10);
192+
if (*s)
193+
return error(_("%s expects a numerical value"),
194+
optname(opt, flags));
195+
if (errno == ERANGE)
196+
return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
197+
arg, optname(opt, flags), lower_bound, upper_bound);
198+
if (errno)
199+
return error_errno(_("value %s for %s cannot be parsed"),
200+
arg, optname(opt, flags));
201+
}
188202

189-
errno = 0;
190-
*(int *)opt->value = strtol(arg, (char **)&s, 10);
191-
if (*s)
192-
return error(_("%s expects a numerical value"),
193-
optname(opt, flags));
194-
if (errno == ERANGE)
203+
if (value < lower_bound || value > upper_bound)
195204
return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
196-
arg, optname(opt, flags), (intmax_t)LONG_MIN, (intmax_t)LONG_MAX);
197-
if (errno)
198-
return error_errno(_("value %s for %s cannot be parsed"),
199-
arg, optname(opt, flags));
205+
arg, optname(opt, flags), lower_bound, upper_bound);
200206

201-
return 0;
207+
switch (opt->precision) {
208+
case 1:
209+
*(int8_t *)opt->value = value;
210+
return 0;
211+
case 2:
212+
*(int16_t *)opt->value = value;
213+
return 0;
214+
case 4:
215+
*(int32_t *)opt->value = value;
216+
return 0;
217+
case 8:
218+
*(int64_t *)opt->value = value;
219+
return 0;
220+
default:
221+
BUG("invalid precision for option %s",
222+
optname(opt, flags));
223+
}
224+
}
202225
case OPTION_MAGNITUDE:
203226
if (unset) {
204227
*(unsigned long *)opt->value = 0;

‎parse-options.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,10 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
9292
* `value`::
9393
* stores pointers to the values to be filled.
9494
*
95+
* `precision`::
96+
* precision of the integer pointed to by `value` in number of bytes. Should
97+
* typically be its `sizeof()`.
98+
*
9599
* `argh`::
96100
* token to explain the kind of argument this option wants. Does not
97101
* begin in capital letter, and does not end with a full stop.
@@ -151,6 +155,7 @@ struct option {
151155
int short_name;
152156
const char *long_name;
153157
void *value;
158+
size_t precision;
154159
const char *argh;
155160
const char *help;
156161

@@ -214,6 +219,7 @@ struct option {
214219
.short_name = (s), \
215220
.long_name = (l), \
216221
.value = (v), \
222+
.precision = sizeof(*v), \
217223
.argh = N_("n"), \
218224
.help = (h), \
219225
.flags = (f), \

‎t/helper/test-parse-options.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ int cmd__parse_options(int argc, const char **argv)
120120
};
121121
struct string_list expect = STRING_LIST_INIT_NODUP;
122122
struct string_list list = STRING_LIST_INIT_NODUP;
123+
int16_t i16 = 0;
123124

124125
struct option options[] = {
125126
OPT_BOOL(0, "yes", &boolean, "get a boolean"),
@@ -139,6 +140,7 @@ int cmd__parse_options(int argc, const char **argv)
139140
OPT_NEGBIT(0, "neg-or4", &boolean, "same as --no-or4", 4),
140141
OPT_GROUP(""),
141142
OPT_INTEGER('i', "integer", &integer, "get a integer"),
143+
OPT_INTEGER(0, "i16", &i16, "get a 16 bit integer"),
142144
OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
143145
OPT_MAGNITUDE('m', "magnitude", &magnitude, "get a magnitude"),
144146
OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
@@ -210,6 +212,7 @@ int cmd__parse_options(int argc, const char **argv)
210212
}
211213
show(&expect, &ret, "boolean: %d", boolean);
212214
show(&expect, &ret, "integer: %d", integer);
215+
show(&expect, &ret, "i16: %"PRIdMAX, (intmax_t) i16);
213216
show(&expect, &ret, "magnitude: %lu", magnitude);
214217
show(&expect, &ret, "timestamp: %"PRItime, timestamp);
215218
show(&expect, &ret, "string: %s", string ? string : "(not set)");

‎t/t0040-parse-options.sh‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ usage: test-tool parse-options <options>
2222
2323
-i, --[no-]integer <n>
2424
get a integer
25+
--[no-]i16 <n> get a 16 bit integer
2526
-j <n> get a integer, too
2627
-m, --magnitude <n> get a magnitude
2728
--[no-]set23 set integer to 23
@@ -136,6 +137,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' '
136137
cat >expect <<\EOF
137138
boolean: 2
138139
integer: 1729
140+
i16: 0
139141
magnitude: 16384
140142
timestamp: 0
141143
string: 123
@@ -156,6 +158,7 @@ test_expect_success 'short options' '
156158
cat >expect <<\EOF
157159
boolean: 2
158160
integer: 1729
161+
i16: 9000
159162
magnitude: 16384
160163
timestamp: 0
161164
string: 321
@@ -167,7 +170,7 @@ file: prefix/fi.le
167170
EOF
168171

169172
test_expect_success 'long options' '
170-
test-tool parse-options --boolean --integer 1729 --magnitude 16k \
173+
test-tool parse-options --boolean --integer 1729 --i16 9000 --magnitude 16k \
171174
--boolean --string2=321 --verbose --verbose --no-dry-run \
172175
--abbrev=10 --file fi.le --obsolete \
173176
>output 2>output.err &&
@@ -179,6 +182,7 @@ test_expect_success 'abbreviate to something longer than SHA1 length' '
179182
cat >expect <<-EOF &&
180183
boolean: 0
181184
integer: 0
185+
i16: 0
182186
magnitude: 0
183187
timestamp: 0
184188
string: (not set)
@@ -253,6 +257,7 @@ test_expect_success 'superfluous value provided: cmdmode' '
253257
cat >expect <<\EOF
254258
boolean: 1
255259
integer: 13
260+
i16: 0
256261
magnitude: 0
257262
timestamp: 0
258263
string: 123
@@ -276,6 +281,7 @@ test_expect_success 'intermingled arguments' '
276281
cat >expect <<\EOF
277282
boolean: 0
278283
integer: 2
284+
i16: 0
279285
magnitude: 0
280286
timestamp: 0
281287
string: (not set)
@@ -343,6 +349,7 @@ cat >expect <<\EOF
343349
Callback: "four", 0
344350
boolean: 5
345351
integer: 4
352+
i16: 0
346353
magnitude: 0
347354
timestamp: 0
348355
string: (not set)
@@ -368,6 +375,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
368375
cat >expect <<\EOF
369376
boolean: 1
370377
integer: 23
378+
i16: 0
371379
magnitude: 0
372380
timestamp: 0
373381
string: (not set)
@@ -447,6 +455,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
447455
cat >expect <<\EOF
448456
boolean: 0
449457
integer: 0
458+
i16: 0
450459
magnitude: 0
451460
timestamp: 0
452461
string: (not set)
@@ -789,4 +798,16 @@ test_expect_success 'overflowing integer' '
789798
test_must_be_empty out
790799
'
791800

801+
test_expect_success 'i16 limits range' '
802+
test-tool parse-options --i16 32767 >out &&
803+
test_grep "i16: 32767" out &&
804+
test_must_fail test-tool parse-options --i16 32768 2>err &&
805+
test_grep "value 32768 for option .i16. not in range \[-32768,32767\]" err &&
806+
807+
test-tool parse-options --i16 -32768 >out &&
808+
test_grep "i16: -32768" out &&
809+
test_must_fail test-tool parse-options --i16 -32769 2>err &&
810+
test_grep "value -32769 for option .i16. not in range \[-32768,32767\]" err
811+
'
812+
792813
test_done

0 commit comments

Comments
 (0)