Re: [PATCH 1/4] Fix maybe-uninitialized warning with GCC at -O3
On Wed, Jun 04, 2025 at 09:36:03AM +0200, Patrick Steinhardt wrote:
Show 14 quoted lines
> On Wed, Jun 04, 2025 at 08:06:43AM +0900, Mike Hommey wrote:
> > ```
> > In file included from parse-options.c:1:
> > git-compat-util.h: In function ‘get_value’:
> > git-compat-util.h:489:21: error: ‘arg’ may be used uninitialized [-Werror=maybe-uninitialized]
> > 489 | #define error(...) (error(__VA_ARGS__), const_error())
> > | ^~~~~
> > parse-options.c:76:21: note: ‘arg’ was declared here
> > 76 | const char *arg;
> > | ^~~
> > ```
>
> A bit more explanation whether this warning is a false positive or
> whether this may be an actual issue would be welcome.
I thought at first it was a false positive, as we'd always fill in "arg" via get_arg(), or return an error. But I suspect the culprit is the earlier part of this if/else chain:
if (unset) {
value = 0;
} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
value = opt->defval;
} else if (get_arg(p, opt, flags, &arg)) {
return -1;
} ...So if "unset" is true, we set "value" but not "arg". The code the compiler is complaining about is here:
if (value < lower_bound)
return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);In the case of "unset", I think we could never trigger this, since our lower bound will never be above 0.
But what about opt->defval? We don't know anything about it here. Probably it would be a programming error to pass in a value that is outside the bounds, but this code doesn't know that. So something like this (it was actually hard to find an integer option with OPTARG!):
Show changes to 2 files +9 −7
builtin/fmt-merge-msg.c, parse-options.c
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3b6aac2cf7..630403611b 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -28,7 +28,7 @@ int cmd_fmt_merge_msg(int argc,
.argh = N_("n"),
.help = N_("populate log with at most <n> entries from shortlog"),
.flags = PARSE_OPT_OPTARG,
- .defval = DEFAULT_MERGE_LOG_LEN,
+ .defval = -2147483649,
},
{
.type = OPTION_INTEGER,
would cause:
git fmt-merge-msg --log
to print uninitialized memory. But it is equally wrong with Mike's
patch; we'd just segfault!
I think this is unlikely to happen in practice, but it does feel like we
should be able to solve it in a better way. I came up with two options.
One is to BUG() on a bogus default value, like this:
diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef..7ddf213d0c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -73,7 +73,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
enum opt_parsed flags,
const char **argp)
{
- const char *arg;
+ const char *arg = NULL;
const int unset = flags & OPT_UNSET;
int err;
@@ -195,9 +195,13 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
optname(opt, flags));
}
- if (value < lower_bound)
+ if (value < lower_bound) {
+ if (!arg)
+ BUG("default option value for '%s' is not in range",
+ optname(opt, flags));
return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);
+ }
switch (opt->precision) {
case 1:
The other is to not bother checking the bounds on the default values at
all, by pushing this bounds check into the if/else chain after we know
we have something to parse, like this (extended context to make it more
obvious how this fits into the chain):
diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef..d36e56da15 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -179,39 +179,38 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
if (unset) {
value = 0;
} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
value = opt->defval;
} else if (get_arg(p, opt, flags, &arg)) {
return -1;
} else if (!*arg) {
return error(_("%s expects a numerical value"),
optname(opt, flags));
} 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));
- }
-
- if (value < lower_bound)
+ } else if (value < lower_bound) {
return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)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));
-Peff
Mike Hommey
