| Message ID | 20231207071132.GF1276005@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1b274c98341ef17f3bbfe80f603f629e7c950668 |
| Headers | show |
| Series | fix segfaults with implicit-bool config | expand |
On Thu, Dec 07, 2023 at 02:11:32AM -0500, Jeff King wrote: > When parsing the "key", "command", and "cmd" trailer config, we just > make a copy of the value string. If we see an implicit bool like: > > [trailer "foo"] > key > > we'll segfault trying to copy a NULL pointer. We can fix this with the > usual config_error_nonbool() check. > > I split this out from the other vanilla cases, because at first glance > it looks like a better fix here would be to move the NULL check out of > the switch statement. But it would change the behavior of other keys > like trailer.*.ifExists, where an implicit bool is interpreted as > EXISTS_DEFAULT. > > Signed-off-by: Jeff King <peff@peff.net> > --- > trailer.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/trailer.c b/trailer.c > index b0e2ec224a..e4b08ed267 100644 > --- a/trailer.c > +++ b/trailer.c > @@ -553,16 +553,22 @@ static int git_trailer_config(const char *conf_key, const char *value, > case TRAILER_KEY: > if (conf->key) > warning(_("more than one %s"), conf_key); > + if (!value) > + return config_error_nonbool(conf_key); > conf->key = xstrdup(value); > break; > case TRAILER_COMMAND: > if (conf->command) > warning(_("more than one %s"), conf_key); > + if (!value) > + return config_error_nonbool(conf_key); > conf->command = xstrdup(value); > break; > case TRAILER_CMD: > if (conf->cmd) > warning(_("more than one %s"), conf_key); > + if (!value) > + return config_error_nonbool(conf_key); > conf->cmd = xstrdup(value); > break; > case TRAILER_WHERE: For the other cases we only generate warnings for unknown values, but return successfully. Should we do the same here? Patrick
diff --git a/trailer.c b/trailer.c index b0e2ec224a..e4b08ed267 100644 --- a/trailer.c +++ b/trailer.c @@ -553,16 +553,22 @@ static int git_trailer_config(const char *conf_key, const char *value, case TRAILER_KEY: if (conf->key) warning(_("more than one %s"), conf_key); + if (!value) + return config_error_nonbool(conf_key); conf->key = xstrdup(value); break; case TRAILER_COMMAND: if (conf->command) warning(_("more than one %s"), conf_key); + if (!value) + return config_error_nonbool(conf_key); conf->command = xstrdup(value); break; case TRAILER_CMD: if (conf->cmd) warning(_("more than one %s"), conf_key); + if (!value) + return config_error_nonbool(conf_key); conf->cmd = xstrdup(value); break; case TRAILER_WHERE:
When parsing the "key", "command", and "cmd" trailer config, we just make a copy of the value string. If we see an implicit bool like: [trailer "foo"] key we'll segfault trying to copy a NULL pointer. We can fix this with the usual config_error_nonbool() check. I split this out from the other vanilla cases, because at first glance it looks like a better fix here would be to move the NULL check out of the switch statement. But it would change the behavior of other keys like trailer.*.ifExists, where an implicit bool is interpreted as EXISTS_DEFAULT. Signed-off-by: Jeff King <peff@peff.net> --- trailer.c | 6 ++++++ 1 file changed, 6 insertions(+)