@matheusbsbs / matheus-bagliano / commits / 791aeddfa2

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>

Patrick Steinhardt committed Apr 17, 2025 at 12:49 UTC 791aeddfa2fdb9e830e24c50c97bb5e8bf3613e6

6 files changed +16 -9

apply.c

+2 -2

@@ -5123,8 +5123,8 @@ int apply_parse_options(int argc, const char **argv,

5123 /* Think twice before adding "--nul" synonym to this */

5124 OPT_SET_INT('z', NULL, &state->line_termination,

5125 N_("paths are separated with NUL character"), '\0'),

5126 - OPT_INTEGER('C', NULL, &state->p_context,

5127 - N_("ensure at least <n> lines of context match")),

5126 + OPT_UNSIGNED('C', NULL, &state->p_context,

5127 + N_("ensure at least <n> lines of context match")),

5128 OPT_CALLBACK(0, "whitespace", state, N_("action"),

5129 N_("detect new or modified lines that have whitespace errors"),

5130 apply_option_parse_whitespace),

builtin/backfill.c

+2 -2

@@ -123,8 +123,8 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit

123 .sparse = 0,

124 };

125 struct option options[] = {

126 - OPT_INTEGER(0, "min-batch-size", &ctx.min_batch_size,

127 - N_("Minimum number of objects to request at a time")),

126 + OPT_UNSIGNED(0, "min-batch-size", &ctx.min_batch_size,

127 + N_("Minimum number of objects to request at a time")),

128 OPT_BOOL(0, "sparse", &ctx.sparse,

129 N_("Restrict the missing objects to the current sparse-checkout")),

130 OPT_END(),

builtin/column.c

+1 -1

@@ -31,7 +31,7 @@ int cmd_column(int argc,

31 struct option options[] = {

32 OPT_STRING(0, "command", &real_command, N_("name"), N_("lookup config vars")),

33 OPT_COLUMN(0, "mode", &colopts, N_("layout to use")),

34 - OPT_INTEGER(0, "raw-mode", &colopts, N_("layout to use")),

34 + OPT_UNSIGNED(0, "raw-mode", &colopts, N_("layout to use")),

35 OPT_INTEGER(0, "width", &copts.width, N_("maximum width")),

36 OPT_STRING(0, "indent", &copts.indent, N_("string"), N_("padding space on left border")),

37 OPT_STRING(0, "nl", &copts.nl, N_("string"), N_("padding space on right border")),

builtin/grep.c

+2 -2

@@ -983,9 +983,9 @@ int cmd_grep(int argc,

983 OPT_CALLBACK('C', "context", &opt, N_("n"),

984 N_("show <n> context lines before and after matches"),

985 context_callback),

986 - OPT_INTEGER('B', "before-context", &opt.pre_context,

986 + OPT_UNSIGNED('B', "before-context", &opt.pre_context,

987 N_("show <n> context lines before matches")),

988 - OPT_INTEGER('A', "after-context", &opt.post_context,

988 + OPT_UNSIGNED('A', "after-context", &opt.post_context,

989 N_("show <n> context lines after matches")),

990 OPT_INTEGER(0, "threads", &num_threads,

991 N_("use <n> worker threads")),

git-compat-util.h

@@ -110,12 +110,19 @@ DISABLE_WARNING(-Wsign-compare)

110 # define BARF_UNLESS_COPYABLE(dst, src) \

111 BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(dst)), \

112 __typeof__(*(src))))

113 +

114 +# define BARF_UNLESS_SIGNED(var) BUILD_ASSERT_OR_ZERO(((__typeof__(var)) -1) < 0)

115 +# define BARF_UNLESS_UNSIGNED(var) BUILD_ASSERT_OR_ZERO(((__typeof__(var)) -1) > 0)

116 #else

117 # define BARF_UNLESS_AN_ARRAY(arr) 0

118 # define BARF_UNLESS_COPYABLE(dst, src) \

119 BUILD_ASSERT_OR_ZERO(0 ? ((*(dst) = *(src)), 0) : \

120 sizeof(*(dst)) == sizeof(*(src)))

121 +

122 +# define BARF_UNLESS_SIGNED(var) 0

123 +# define BARF_UNLESS_UNSIGNED(var) 0

124 #endif

125 +

126 /*

127 * ARRAY_SIZE - get the number of elements in a visible array

128 * @x: the array whose size you want.

parse-options.h

+2 -2

@@ -218,7 +218,7 @@ struct option {

218 .type = OPTION_INTEGER, \

219 .short_name = (s), \

220 .long_name = (l), \

221 - .value = (v), \

221 + .value = (v) + BARF_UNLESS_SIGNED(*(v)), \

222 .precision = sizeof(*v), \

223 .argh = N_("n"), \

224 .help = (h), \

@@ -280,7 +280,7 @@ struct option {

280 .type = OPTION_UNSIGNED, \

281 .short_name = (s), \

282 .long_name = (l), \

283 - .value = (v), \

283 + .value = (v) + BARF_UNLESS_UNSIGNED(*(v)), \

284 .precision = sizeof(*v), \

285 .argh = N_("n"), \

286 .help = (h), \