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>

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 240cdb474bc49b0cd686c3cf5ed3bc6b786ede2e..3b6aac2cf7faab219ff171a5f5ae375349bfc1f7 100644 (file)
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -24,6 +24,7 @@ int cmd_fmt_merge_msg(int argc,
                        .type = OPTION_INTEGER,
                        .long_name = "log",
                        .value = &shortlog_len,
+                       .precision = sizeof(shortlog_len),
                        .argh = N_("n"),
                        .help = N_("populate log with at most <n> entries from shortlog"),
                        .flags = PARSE_OPT_OPTARG,
@@ -33,6 +34,7 @@ int cmd_fmt_merge_msg(int argc,
                        .type = OPTION_INTEGER,
                        .long_name = "summary",
                        .value = &shortlog_len,
+                       .precision = sizeof(shortlog_len),
                        .argh = N_("n"),
                        .help = N_("alias for --log (deprecated)"),
                        .flags = PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN,

diff --git a/builtin/merge.c b/builtin/merge.c
index 21787d45165e6d1907a4980be87a8a3f0e820b90..9ab10c7db0a63783c138cdab6f3966204a3bfbf7 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -254,6 +254,7 @@ static struct option builtin_merge_options[] = {
                .type = OPTION_INTEGER,
                .long_name = "log",
                .value = &shortlog_len,
+               .precision = sizeof(shortlog_len),
                .argh = N_("n"),
                .help = N_("add (at most <n>) entries from shortlog to merge commit message"),
                .flags = PARSE_OPT_OPTARG,

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index dab37019d29210bf40c5bf3bd3210ce34a100eb3..b549d8c3f5b8609124e3fed8f083ff021b91083c 100644 (file)
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -671,6 +671,7 @@ int cmd_show_branch(int ac,
                        .type = OPTION_INTEGER,
                        .long_name = "more",
                        .value = &extra,
+                       .precision = sizeof(extra),
                        .argh = N_("n"),
                        .help = N_("show <n> more commits after the common ancestor"),
                        .flags = PARSE_OPT_OPTARG,

diff --git a/builtin/tag.c b/builtin/tag.c
index b266f12bb48d12f2fb6eee815f2c28959f9a188d..7597d93c71bd950192cb036156c7a76157329d56 100644 (file)
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -483,6 +483,7 @@ int cmd_tag(int argc,
                        .type = OPTION_INTEGER,
                        .short_name = 'n',
                        .value = &filter.lines,
+                       .precision = sizeof(filter.lines),
                        .argh = N_("n"),
                        .help = N_("print <n> lines of each tag message"),
                        .flags = PARSE_OPT_OPTARG,

diff --git a/parse-options.c b/parse-options.c
index d23e587e98bd975275076c918fee1e0e2b5616a4..768718a397242e90887f72efe4cd7a62842c2633 100644 (file)
--- a/parse-options.c
+++ b/parse-options.c
@@ -172,25 +172,51 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
                        return (*opt->ll_callback)(p, opt, p_arg, p_unset);
        }
        case OPTION_INTEGER:
+       {
+               intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - CHAR_BIT * opt->precision);
+               intmax_t lower_bound = -upper_bound - 1;
+               intmax_t value;
+
                if (unset) {
-                       *(int *)opt->value = 0;
-                       return 0;
-               }
-               if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
-                       *(int *)opt->value = opt->defval;
-                       return 0;
-               }
-               if (get_arg(p, opt, flags, &arg))
+                       value = 0;
+               } else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
+                       value = opt->defval;
+               } else if (get_arg(p, opt, flags, &arg)) {
                        return -1;
-               if (!*arg)
+               } else if (!*arg) {
                        return error(_("%s expects a numerical value"),
                                     optname(opt, flags));
-               if (!git_parse_int(arg, opt->value))
-                       return error(_("%s expects an integer value"
-                                      " with an optional k/m/g suffix"),
+               } else if (!git_parse_signed(arg, &value, upper_bound)) {
+                       if (errno == ERANGE)
+                               return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
+                                            arg, optname(opt, flags), lower_bound, upper_bound);
+
+                       return error(_("%s expects an integer value with an optional k/m/g suffix"),
                                     optname(opt, flags));
-               return 0;
+               }
+
+               if (value < lower_bound)
+                       return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
+                                    arg, optname(opt, flags), lower_bound, upper_bound);
 
+               switch (opt->precision) {
+               case 1:
+                       *(int8_t *)opt->value = value;
+                       return 0;
+               case 2:
+                       *(int16_t *)opt->value = value;
+                       return 0;
+               case 4:
+                       *(int32_t *)opt->value = value;
+                       return 0;
+               case 8:
+                       *(int64_t *)opt->value = value;
+                       return 0;
+               default:
+                       BUG("invalid precision for option %s",
+                           optname(opt, flags));
+               }
+       }
        case OPTION_UNSIGNED:
                if (unset) {
                        *(unsigned long *)opt->value = 0;

diff --git a/parse-options.h b/parse-options.h
index 14e4df1ee214795515f94e99bec6821ff9e3e081..4c430c7273c13eebfe9fb1855f0d64df3e05f237 100644 (file)
--- a/parse-options.h
+++ b/parse-options.h
@@ -92,6 +92,10 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
  * `value`::
  *   stores pointers to the values to be filled.
  *
+ * `precision`::
+ *   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
  *   begin in capital letter, and does not end with a full stop.
@@ -151,6 +155,7 @@ struct option {
        int short_name;
        const char *long_name;
        void *value;
+       size_t precision;
        const char *argh;
        const char *help;
 
@@ -214,6 +219,7 @@ struct option {
        .short_name = (s), \
        .long_name = (l), \
        .value = (v), \
+       .precision = sizeof(*v), \
        .argh = N_("n"), \
        .help = (h), \
        .flags = (f), \

diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index fc3e2861c26af3268629a1263e6b45567173171c..3689aee831521f34cefd6367f676d95a80e685e0 100644 (file)
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -120,6 +120,7 @@ int cmd__parse_options(int argc, const char **argv)
        };
        struct string_list expect = STRING_LIST_INIT_NODUP;
        struct string_list list = STRING_LIST_INIT_NODUP;
+       int16_t i16 = 0;
 
        struct option options[] = {
                OPT_BOOL(0, "yes", &boolean, "get a boolean"),
@@ -139,6 +140,7 @@ int cmd__parse_options(int argc, const char **argv)
                OPT_NEGBIT(0, "neg-or4", &boolean, "same as --no-or4", 4),
                OPT_GROUP(""),
                OPT_INTEGER('i', "integer", &integer, "get a integer"),
+               OPT_INTEGER(0, "i16", &i16, "get a 16 bit integer"),
                OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
                OPT_UNSIGNED('u', "unsigned", &unsigned_integer, "get an unsigned integer"),
                OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
@@ -210,6 +212,7 @@ int cmd__parse_options(int argc, const char **argv)
        }
        show(&expect, &ret, "boolean: %d", boolean);
        show(&expect, &ret, "integer: %d", integer);
+       show(&expect, &ret, "i16: %"PRIdMAX, (intmax_t) i16);
        show(&expect, &ret, "unsigned: %lu", unsigned_integer);
        show(&expect, &ret, "timestamp: %"PRItime, timestamp);
        show(&expect, &ret, "string: %s", string ? string : "(not set)");

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 65a11c8dbc8c86054a26326eb1acdd35c67e1904..be785547eaddea3ff53d73ece6cfb51aa3846764 100755 (executable)
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -22,6 +22,7 @@ usage: test-tool parse-options <options>
 
     -i, --[no-]integer <n>
                           get a integer
+    --[no-]i16 <n>        get a 16 bit integer
     -j <n>                get a integer, too
     -u, --unsigned <n>    get an unsigned integer
     --[no-]set23          set integer to 23
@@ -138,6 +139,7 @@ test_expect_success 'OPT_UNSIGNED() 3giga' '
 cat >expect <<\EOF
 boolean: 2
 integer: 1729
+i16: 0
 unsigned: 16384
 timestamp: 0
 string: 123
@@ -158,6 +160,7 @@ test_expect_success 'short options' '
 cat >expect <<\EOF
 boolean: 2
 integer: 1729
+i16: 9000
 unsigned: 16384
 timestamp: 0
 string: 321
@@ -169,7 +172,7 @@ file: prefix/fi.le
 EOF
 
 test_expect_success 'long options' '
-       test-tool parse-options --boolean --integer 1729 --unsigned 16k \
+       test-tool parse-options --boolean --integer 1729 --i16 9000 --unsigned 16k \
                --boolean --string2=321 --verbose --verbose --no-dry-run \
                --abbrev=10 --file fi.le --obsolete \
                >output 2>output.err &&
@@ -181,6 +184,7 @@ test_expect_success 'abbreviate to something longer than SHA1 length' '
        cat >expect <<-EOF &&
        boolean: 0
        integer: 0
+       i16: 0
        unsigned: 0
        timestamp: 0
        string: (not set)
@@ -255,6 +259,7 @@ test_expect_success 'superfluous value provided: cmdmode' '
 cat >expect <<\EOF
 boolean: 1
 integer: 13
+i16: 0
 unsigned: 0
 timestamp: 0
 string: 123
@@ -278,6 +283,7 @@ test_expect_success 'intermingled arguments' '
 cat >expect <<\EOF
 boolean: 0
 integer: 2
+i16: 0
 unsigned: 0
 timestamp: 0
 string: (not set)
@@ -345,6 +351,7 @@ cat >expect <<\EOF
 Callback: "four", 0
 boolean: 5
 integer: 4
+i16: 0
 unsigned: 0
 timestamp: 0
 string: (not set)
@@ -370,6 +377,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
 cat >expect <<\EOF
 boolean: 1
 integer: 23
+i16: 0
 unsigned: 0
 timestamp: 0
 string: (not set)
@@ -449,6 +457,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
 cat >expect <<\EOF
 boolean: 0
 integer: 0
+i16: 0
 unsigned: 0
 timestamp: 0
 string: (not set)
@@ -785,4 +794,16 @@ test_expect_success 'unsigned with units but no numbers' '
        test_must_be_empty out
 '
 
+test_expect_success 'i16 limits range' '
+       test-tool parse-options --i16 32767 >out &&
+       test_grep "i16: 32767" out &&
+       test_must_fail test-tool parse-options --i16 32768 2>err &&
+       test_grep "value 32768 for option .i16. not in range \[-32768,32767\]" err &&
+
+       test-tool parse-options --i16 -32768 >out &&
+       test_grep "i16: -32768" out &&
+       test_must_fail test-tool parse-options --i16 -32769 2>err &&
+       test_grep "value -32769 for option .i16. not in range \[-32768,32767\]" err
+'
+
 test_done