Skip to content

Commit 0970569

Browse files
pks-tgitster
authored andcommitted
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 785c17d commit 0970569

File tree

8 files changed

+75
-14
lines changed

8 files changed

+75
-14
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: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -172,25 +172,51 @@ 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));
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"),
191195
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);
193201

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+
}
194220
case OPTION_UNSIGNED:
195221
if (unset) {
196222
*(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_UNSIGNED('u', "unsigned", &unsigned_integer, "get an unsigned integer"),
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, "unsigned: %lu", unsigned_integer);
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
-u, --unsigned <n> get an unsigned integer
2728
--[no-]set23 set integer to 23
@@ -138,6 +139,7 @@ test_expect_success 'OPT_UNSIGNED() 3giga' '
138139
cat >expect <<\EOF
139140
boolean: 2
140141
integer: 1729
142+
i16: 0
141143
unsigned: 16384
142144
timestamp: 0
143145
string: 123
@@ -158,6 +160,7 @@ test_expect_success 'short options' '
158160
cat >expect <<\EOF
159161
boolean: 2
160162
integer: 1729
163+
i16: 9000
161164
unsigned: 16384
162165
timestamp: 0
163166
string: 321
@@ -169,7 +172,7 @@ file: prefix/fi.le
169172
EOF
170173

171174
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 \
173176
--boolean --string2=321 --verbose --verbose --no-dry-run \
174177
--abbrev=10 --file fi.le --obsolete \
175178
>output 2>output.err &&
@@ -181,6 +184,7 @@ test_expect_success 'abbreviate to something longer than SHA1 length' '
181184
cat >expect <<-EOF &&
182185
boolean: 0
183186
integer: 0
187+
i16: 0
184188
unsigned: 0
185189
timestamp: 0
186190
string: (not set)
@@ -255,6 +259,7 @@ test_expect_success 'superfluous value provided: cmdmode' '
255259
cat >expect <<\EOF
256260
boolean: 1
257261
integer: 13
262+
i16: 0
258263
unsigned: 0
259264
timestamp: 0
260265
string: 123
@@ -278,6 +283,7 @@ test_expect_success 'intermingled arguments' '
278283
cat >expect <<\EOF
279284
boolean: 0
280285
integer: 2
286+
i16: 0
281287
unsigned: 0
282288
timestamp: 0
283289
string: (not set)
@@ -345,6 +351,7 @@ cat >expect <<\EOF
345351
Callback: "four", 0
346352
boolean: 5
347353
integer: 4
354+
i16: 0
348355
unsigned: 0
349356
timestamp: 0
350357
string: (not set)
@@ -370,6 +377,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
370377
cat >expect <<\EOF
371378
boolean: 1
372379
integer: 23
380+
i16: 0
373381
unsigned: 0
374382
timestamp: 0
375383
string: (not set)
@@ -449,6 +457,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
449457
cat >expect <<\EOF
450458
boolean: 0
451459
integer: 0
460+
i16: 0
452461
unsigned: 0
453462
timestamp: 0
454463
string: (not set)
@@ -785,4 +794,16 @@ test_expect_success 'unsigned with units but no numbers' '
785794
test_must_be_empty out
786795
'
787796

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+
788809
test_done

0 commit comments

Comments
 (0)