parse-options: detect mismatches in integer signedness

It was reported that "t5620-backfill.sh" fails on s390x and sparc64 in a
test that exercises the "--min-batch-size" command line option. The
symptom was that the option didn't seem to have an effect: we didn't
fetch objects with a batch size of 20, but instead fetched all objects
at once.

As it turns out, the root cause is that `--min-batch-size` uses
`OPT_INTEGER()` to parse the command line option. While this macro
expects the caller to pass a pointer to an integer, we instead pass a
pointer to a `size_t`. This coincidentally works on most platforms, but
it breaks apart on the mentioned platforms because they are big endian.

This issue isn't specific to git-backfill(1): there are a couple of
other places where we have the same type confusion going on. This
indicates that the issue really is the interface that the parse-options
subsystem provides -- it is simply too easy to get this wrong as there
isn't any kind of compiler warning, and things just work on the most
common systems.

Address the systemic issue by introducing two new build asserts
`BARF_UNLESS_SIGNED()` and `BARF_UNLESS_UNSIGNED()`. As the names
already hint at, those macros will cause a compiler error when passed a
value that is not signed or unsigned, respectively.

Adapt `OPT_INTEGER()`, `OPT_UNSIGNED()` as well as `OPT_MAGNITUDE()` to
use those asserts. This uncovers a small set of sites where we indeed
have the same bug as in git-backfill(1). Adapt all of them to use the
correct option.

Reported-by: Todd Zullinger <tmz@pobox.com>
Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Helped-by: SZEDER Gábor <szeder.dev@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/apply.c b/apply.c
index f274a3794877dc110c5def32de995c6dfb0d48f9..a850c7d75fe25e1a340f7de15f00667b969f1e6b 100644 (file)
--- a/apply.c
+++ b/apply.c
@@ -5123,8 +5123,8 @@ int apply_parse_options(int argc, const char **argv,
                /* Think twice before adding "--nul" synonym to this */
                OPT_SET_INT('z', NULL, &state->line_termination,
                        N_("paths are separated with NUL character"), '\0'),
-               OPT_INTEGER('C', NULL, &state->p_context,
-                               N_("ensure at least <n> lines of context match")),
+               OPT_UNSIGNED('C', NULL, &state->p_context,
+                            N_("ensure at least <n> lines of context match")),
                OPT_CALLBACK(0, "whitespace", state, N_("action"),
                        N_("detect new or modified lines that have whitespace errors"),
                        apply_option_parse_whitespace),

diff --git a/builtin/backfill.c b/builtin/backfill.c
index 33e1ea2f84ff6b09ef99408d6878808bb6c23017..d95d7a2d4d698be2f3baae0e5ad843948f537606 100644 (file)
--- a/builtin/backfill.c
+++ b/builtin/backfill.c
@@ -123,8 +123,8 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
                .sparse = 0,
        };
        struct option options[] = {
-               OPT_INTEGER(0, "min-batch-size", &ctx.min_batch_size,
-                           N_("Minimum number of objects to request at a time")),
+               OPT_UNSIGNED(0, "min-batch-size", &ctx.min_batch_size,
+                            N_("Minimum number of objects to request at a time")),
                OPT_BOOL(0, "sparse", &ctx.sparse,
                         N_("Restrict the missing objects to the current sparse-checkout")),
                OPT_END(),

diff --git a/builtin/column.c b/builtin/column.c
index 50314cc2559e55bae712ad59af849b8816c1dc2e..ce6443d5fac84d60e2bf906dfc91a8a86782216a 100644 (file)
--- a/builtin/column.c
+++ b/builtin/column.c
@@ -31,7 +31,7 @@ int cmd_column(int argc,
        struct option options[] = {
                OPT_STRING(0, "command", &real_command, N_("name"), N_("lookup config vars")),
                OPT_COLUMN(0, "mode", &colopts, N_("layout to use")),
-               OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")),
+               OPT_UNSIGNED(0, "raw-mode", &colopts, N_("layout to use")),
                OPT_INTEGER(0, "width", &copts.width, N_("maximum width")),
                OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")),
                OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")),

diff --git a/builtin/grep.c b/builtin/grep.c
index c4869733e1bab6e2e73cd4a7121c5f5c5ed6e7ae..f23a6f1dc865ca0860685564b9c8477a32f099ee 100644 (file)
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -983,9 +983,9 @@ int cmd_grep(int argc,
                OPT_CALLBACK('C', "context", &opt, N_("n"),
                        N_("show <n> context lines before and after matches"),
                        context_callback),
-               OPT_INTEGER('B', "before-context", &opt.pre_context,
+               OPT_UNSIGNED('B', "before-context", &opt.pre_context,
                        N_("show <n> context lines before matches")),
-               OPT_INTEGER('A', "after-context", &opt.post_context,
+               OPT_UNSIGNED('A', "after-context", &opt.post_context,
                        N_("show <n> context lines after matches")),
                OPT_INTEGER(0, "threads", &num_threads,
                        N_("use <n> worker threads")),

diff --git a/git-compat-util.h b/git-compat-util.h
index cf733b38acdea071644c0f9181cd69e7416bf89d..1218fcf81a4b564640348087ecdabdcdc0546f80 100644 (file)
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -110,12 +110,19 @@ DISABLE_WARNING(-Wsign-compare)
 # define BARF_UNLESS_COPYABLE(dst, src) \
        BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(dst)), \
                                                          __typeof__(*(src))))
+
+# define BARF_UNLESS_SIGNED(var)   BUILD_ASSERT_OR_ZERO(((__typeof__(var)) -1) < 0)
+# define BARF_UNLESS_UNSIGNED(var) BUILD_ASSERT_OR_ZERO(((__typeof__(var)) -1) > 0)
 #else
 # define BARF_UNLESS_AN_ARRAY(arr) 0
 # define BARF_UNLESS_COPYABLE(dst, src) \
        BUILD_ASSERT_OR_ZERO(0 ? ((*(dst) = *(src)), 0) : \
                                 sizeof(*(dst)) == sizeof(*(src)))
+
+# define BARF_UNLESS_SIGNED(var)   0
+# define BARF_UNLESS_UNSIGNED(var) 0
 #endif
+
 /*
  * ARRAY_SIZE - get the number of elements in a visible array
  * @x: the array whose size you want.

diff --git a/parse-options.h b/parse-options.h
index dc460a26ff1e4c0379c604d2a5996c23fe9942f8..91c3e3c29b3dda1f679ae4c77e250180ed6f9639 100644 (file)
--- a/parse-options.h
+++ b/parse-options.h
@@ -218,7 +218,7 @@ struct option {
        .type = OPTION_INTEGER, \
        .short_name = (s), \
        .long_name = (l), \
-       .value = (v), \
+       .value = (v) + BARF_UNLESS_SIGNED(*(v)), \
        .precision = sizeof(*v), \
        .argh = N_("n"), \
        .help = (h), \
@@ -280,7 +280,7 @@ struct option {
        .type = OPTION_UNSIGNED, \
        .short_name = (s), \
        .long_name = (l), \
-       .value = (v), \
+       .value = (v) + BARF_UNLESS_UNSIGNED(*(v)), \
        .precision = sizeof(*v), \
        .argh = N_("n"), \
        .help = (h), \