* Re: [PATCH 1/7] config: handle NULL value when parsing non-bools
From: Patrick Steinhardt @ 2023-12-07 8:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071114.GA1276005@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 7461 bytes --]
On Thu, Dec 07, 2023 at 02:11:14AM -0500, Jeff King wrote:
> When the config parser sees an "implicit" bool like:
>
> [core]
> someVariable
>
> it passes NULL to the config callback. Any callback code which expects a
> string must check for NULL. This usually happens via helpers like
> git_config_string(), etc, but some custom code forgets to do so and will
> segfault.
>
> These are all fairly vanilla cases where the solution is just the usual
> pattern of:
>
> if (!value)
> return config_error_nonbool(var);
>
> though note that in a few cases we have to split initializers like:
>
> int some_var = initializer();
>
> into:
>
> int some_var;
> if (!value)
> return config_error_nonbool(var);
> some_var = initializer();
>
> There are still some broken instances after this patch, which I'll
> address on their own in individual patches after this one.
>
> Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/blame.c | 2 ++
> builtin/checkout.c | 2 ++
> builtin/clone.c | 2 ++
> builtin/log.c | 5 ++++-
> builtin/pack-objects.c | 6 +++++-
> compat/mingw.c | 2 ++
> config.c | 8 ++++++++
> diff.c | 19 ++++++++++++++++---
> mailinfo.c | 2 ++
> notes-utils.c | 2 ++
> trailer.c | 2 ++
> 11 files changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 9c987d6567..2433b7da5c 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "blame.coloring")) {
> + if (!value)
> + return config_error_nonbool(var);
In the `else` statement where we fail to parse the value we only
generate a warning and return successfully regardless. Should we do the
same here?
> if (!strcmp(value, "repeatedLines")) {
> coloring_mode |= OUTPUT_COLOR_LINE;
> } else if (!strcmp(value, "highlightRecent")) {
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f02434bc15..d5c784854f 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value,
> struct checkout_opts *opts = cb;
>
> if (!strcmp(var, "diff.ignoresubmodules")) {
> + if (!value)
> + return config_error_nonbool(var);
> handle_ignore_submodules_arg(&opts->diff_options, value);
> return 0;
> }
This one is fine, `handle_ignore_submodules_arg()` dies if it gets an
unknown value and `git_config()` will die when the callback function
returns an error. The same is true for many other cases you've
converted.
[snip]
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 89a8b5a976..62c540b4db 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v,
> return 0;
> }
> if (!strcmp(k, "uploadpack.blobpackfileuri")) {
> - struct configured_exclusion *ex = xmalloc(sizeof(*ex));
> + struct configured_exclusion *ex;
> const char *oid_end, *pack_end;
> /*
> * Stores the pack hash. This is not a true object ID, but is
> * of the same form.
> */
> struct object_id pack_hash;
>
> + if (!v)
> + return config_error_nonbool(k);
> +
> + ex = xmalloc(sizeof(*ex));
> if (parse_oid_hex(v, &ex->e.oid, &oid_end) ||
> *oid_end != ' ' ||
> parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||
This isn't part of the diff and not a new issue, but why don't we
`return 0` when parsing this config correctly? We fall through to
`git_default_config()` even if we've successfully parsed the config key,
which seems like a bug to me.
Anyway, this case looks fine.
[snip]
> diff --git a/config.c b/config.c
> index b330c7adb4..18085c7e38 100644
> --- a/config.c
> +++ b/config.c
> @@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value,
> return 0;
> }
> if (!strcmp(var, "core.checkstat")) {
> + if (!value)
> + return config_error_nonbool(var);
> if (!strcasecmp(value, "default"))
> check_stat = 1;
> else if (!strcasecmp(value, "minimal"))
We would ignore `true` here, so should we ignore implicit `true`, as
well?
> @@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "core.checkroundtripencoding")) {
> + if (!value)
> + return config_error_nonbool(var);
> check_roundtrip_encoding = xstrdup(value);
> return 0;
> }
>
> if (!strcmp(var, "core.notesref")) {
> + if (!value)
> + return config_error_nonbool(var);
> notes_ref_name = xstrdup(value);
> return 0;
> }
I wonder the same here. We might as well use `xstrdup_or_null()`, but it
feels like the right thing to do to convert these to actual errors.
> @@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value,
> if (!strcmp(var, "diff.orderfile"))
> return git_config_pathname(&diff_order_file_cfg, var, value);
>
> - if (!strcmp(var, "diff.ignoresubmodules"))
> + if (!strcmp(var, "diff.ignoresubmodules")) {
> + if (!value)
> + return config_error_nonbool(var);
> handle_ignore_submodules_arg(&default_diff_options, value);
> + }
>
> if (!strcmp(var, "diff.submodule")) {
> + if (!value)
> + return config_error_nonbool(var);
> if (parse_submodule_params(&default_diff_options, value))
> warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
> value);
Should we generate a warning instead according to the preexisting code
for "diff.submodule"?
> @@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value,
>
> if (!strcmp(var, "diff.dirstat")) {
> struct strbuf errmsg = STRBUF_INIT;
> + if (!value)
> + return config_error_nonbool(var);
> default_diff_options.dirstat_permille = diff_dirstat_permille_default;
> if (parse_dirstat_params(&default_diff_options, value, &errmsg))
> warning(_("Found errors in 'diff.dirstat' config variable:\n%s"),
Same here, should we generate a warning instead?
> diff --git a/notes-utils.c b/notes-utils.c
> index 97c031c26e..01f4f5b424 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v,
> }
> return 0;
> } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
> + if (!v)
> + return config_error_nonbool(k);
> /* note that a refs/ prefix is implied in the
> * underlying for_each_glob_ref */
> if (starts_with(v, "refs/notes/"))
Here, as well.
> diff --git a/trailer.c b/trailer.c
> index b6de5d9cb2..b0e2ec224a 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
> warning(_("unknown value '%s' for key '%s'"),
> value, conf_key);
> } else if (!strcmp(trailer_item, "separators")) {
> + if (!value)
> + return config_error_nonbool(conf_key);
> separators = xstrdup(value);
> }
> }
And here.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/7] fix segfaults with implicit-bool config
From: Patrick Steinhardt @ 2023-12-07 8:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071030.GA1275835@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]
On Thu, Dec 07, 2023 at 02:10:30AM -0500, Jeff King wrote:
> Carlos reported to the security list a case where you can cause Git
> to segfault by using an implicit bool like:
>
> [core]
> someVariable
>
> when the parsing side for core.someVariable does not correctly check a
> NULL "value" string. This is mostly harmless, as anybody who can feed
> arbitrary config can already execute arbitrary code. There is one case
> of this when parsing .gitmodules (which we don't trust), but even there
> I don't think the security implications are that interesting. A
> malicious repo can get "clone --recurse-submodules" to segfault, but
> always with a strict NULL dereference, not any kind of controllable
> pointer. See patch 5 for more details.
>
> I audited the whole code base for instances of the problem. It was
> fairly manual, so it's possible I missed a spot, but I think this should
> cover everything.
>
> The first patch has vanilla cases, and the rest are instances where I
> thought it was worth calling out specific details.
Thanks for working on this topic! I've left a couple of comments, most
of which are about whether we should retain previous behaviour where we
generate a warning instead of raising an error for unknown values.
Patrick
> [1/7]: config: handle NULL value when parsing non-bools
> [2/7]: setup: handle NULL value when parsing extensions
> [3/7]: trace2: handle NULL values in tr2_sysenv config callback
> [4/7]: help: handle NULL value for alias.* config
> [5/7]: submodule: handle NULL value when parsing submodule.*.branch
> [6/7]: trailer: handle NULL value when parsing trailer-specific config
> [7/7]: fsck: handle NULL value when parsing message config
>
> builtin/blame.c | 2 ++
> builtin/checkout.c | 2 ++
> builtin/clone.c | 2 ++
> builtin/log.c | 5 ++++-
> builtin/pack-objects.c | 6 +++++-
> builtin/receive-pack.c | 11 +++++++----
> compat/mingw.c | 2 ++
> config.c | 8 ++++++++
> diff.c | 19 ++++++++++++++++---
> fetch-pack.c | 12 ++++++++----
> fsck.c | 8 ++++++--
> help.c | 5 ++++-
> mailinfo.c | 2 ++
> notes-utils.c | 2 ++
> setup.c | 2 ++
> submodule-config.c | 4 +++-
> trace2/tr2_sysenv.c | 2 ++
> trailer.c | 8 ++++++++
> 18 files changed, 85 insertions(+), 17 deletions(-)
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] setup: recognize bare repositories with packed-refs
From: Jeff King @ 2023-12-07 7:34 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Adam Majer, git
In-Reply-To: <ZXFt1foDuHKBmFwk@tanuki>
On Thu, Dec 07, 2023 at 08:01:41AM +0100, Patrick Steinhardt wrote:
> > So forgetting at all about how we structure the code, it seems to me
> > that the problem is not new code, but all of the existing code which
> > looks for access("refs", X_OK).
>
> True. The question is of course how much value there is in an old tool
> to be able to discover a new repository that it wouldn't be able to read
> in the first place due to it not understanding the reference format. So
> I'd very much like to see that eventually, we're able to get rid of
> "legacy" cruft that doesn't serve any purpose anymore.
Right, nobody is going to be mad about not being able to use the
repository with old code. My concern is that by skipping it in the
discovery phase, though, the user may see unexpected behavior (like
continuing and finding some _other_ repo). I admit it's a pretty narrow
case, though.
> The question is whether we can do a better job of this going forward so
> that at least we don't have to pose the same question in the future.
> Right now, we'll face the same problem whenever any part of the current
> on-disk repository data structures changes.
>
> I wonder whether it would make sense to introduce something like a
> filesystem-level hint, e.g. in the form of a new ".is-git-repository"
> file. If Git discovers that file then it assumes the directory to be a
> Git repository -- and everything else is set up by parsing the config
> and thus the repository's configured format.
I kind of think the presence of a well-formed HEAD is a good indicator
that it is a Git directory. IOW, I don't have any real problem with
simply loosening is_git_directory() to remove the "refs/" check
entirely. But again, that can only help us going forward; older versions
will still get confused if we truly ditch "refs/" for other formats.
Of course some ref formats may want to avoid the "HEAD" file itself, so
your .is-git-repository would be cleaner. I'm just not sure if it's
worth the headache to try to switch things now.
> > We already have an extension config option to specify that we're using
> > reftable, don't we? But anything in config has the same chicken-and-egg
> > problems as above, I think.
>
> Not yet, no. I plan to submit the new "extensions.refFormat" extension
> soonish though, probably next week.
Ah, OK. I remember talking about it with Han-Wen long ago, but I admit I
have not paid much attention to reftable work recently. :) So I am happy
you are picking it up.
-Peff
^ permalink raw reply
* [PATCH 9/9] sequencer: simplify away extra git_config_string() call
From: Jeff King @ 2023-12-07 7:26 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
In our config callback, we call git_config_string() to copy the incoming
value string into a local string. But we don't modify or store that
string; we just look at it and then free it. We can make the code
simpler by just looking at the value passed into the callback.
Note that we do need to check for NULL, which is the one bit of logic
git_config_string() did for us. And I could even see an argument that we
are abstracting any error-checking of the value behind the
git_config_string() layer. But in practice no other callbacks behave
this way; it is standard to check for NULL and then just look at the
string directly.
Signed-off-by: Jeff King <peff@peff.net>
---
sequencer.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..74c3b1243e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -238,34 +238,29 @@ static int git_sequencer_config(const char *k, const char *v,
const struct config_context *ctx, void *cb)
{
struct replay_opts *opts = cb;
- int status;
if (!strcmp(k, "commit.cleanup")) {
- const char *s;
+ if (!v)
+ return config_error_nonbool(k);
- status = git_config_string(&s, k, v);
- if (status)
- return status;
-
- if (!strcmp(s, "verbatim")) {
+ if (!strcmp(v, "verbatim")) {
opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
opts->explicit_cleanup = 1;
- } else if (!strcmp(s, "whitespace")) {
+ } else if (!strcmp(v, "whitespace")) {
opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
opts->explicit_cleanup = 1;
- } else if (!strcmp(s, "strip")) {
+ } else if (!strcmp(v, "strip")) {
opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
opts->explicit_cleanup = 1;
- } else if (!strcmp(s, "scissors")) {
+ } else if (!strcmp(v, "scissors")) {
opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SCISSORS;
opts->explicit_cleanup = 1;
} else {
warning(_("invalid commit message cleanup mode '%s'"),
- s);
+ v);
}
- free((char *)s);
- return status;
+ return 0;
}
if (!strcmp(k, "commit.gpgsign")) {
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 8/9] gpg-interface: drop pointless config_error_nonbool() checks
From: Jeff King @ 2023-12-07 7:26 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
Config callbacks which use git_config_string() or git_config_pathname()
have no need to check for a NULL value. This is handled automatically
by those helpers.
Signed-off-by: Jeff King <peff@peff.net>
---
gpg-interface.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 48f43c5a21..25c42cb9fd 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -762,23 +762,14 @@ static int git_gpg_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "gpg.ssh.defaultkeycommand")) {
- if (!value)
- return config_error_nonbool(var);
+ if (!strcmp(var, "gpg.ssh.defaultkeycommand"))
return git_config_string(&ssh_default_key_command, var, value);
- }
- if (!strcmp(var, "gpg.ssh.allowedsignersfile")) {
- if (!value)
- return config_error_nonbool(var);
+ if (!strcmp(var, "gpg.ssh.allowedsignersfile"))
return git_config_pathname(&ssh_allowed_signers, var, value);
- }
- if (!strcmp(var, "gpg.ssh.revocationfile")) {
- if (!value)
- return config_error_nonbool(var);
+ if (!strcmp(var, "gpg.ssh.revocationfile"))
return git_config_pathname(&ssh_revocation_file, var, value);
- }
if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
fmtname = "openpgp";
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 7/9] push: drop confusing configset/callback redundancy
From: Jeff King @ 2023-12-07 7:26 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
We parse push config by calling git_config() with our git_push_config()
callback. But inside that callback, when we see "push.gpgsign", we
ignore the value passed into the callback and instead make a new call to
git_config_get_value().
This is unnecessary at best, and slightly wrong at worst (if there are
multiple instances, get_value() only returns one; both methods end up
with last-one-wins, but we'd fail to report errors if earlier
incarnations were bogus).
The call was added by 68c757f219 (push: add a config option push.gpgSign
for default signed pushes, 2015-08-19). That commit doesn't give any
reason to deviate from the usual strategy here; it was probably just
somebody unfamiliar with our config API and its conventions.
It also added identical code to builtin/send-pack.c, which also handles
push.gpgsign.
And then the same issue spread to its neighbor in b33a15b081 (push: add
recurseSubmodules config option, 2015-11-17), presumably via
cargo-culting.
This patch fixes all three to just directly use the value provided to
the callback. While I was adjusting the code to do so, I noticed that
push.gpgsign is overly careful about a NULL value. After
git_parse_maybe_bool() has returned anything besides 1, we know that the
value cannot be NULL (if it were, it would be an implicit "true", and
many callers of maybe_bool rely on that). Here that lets us shorten "if
(v && !strcasecmp(v, ...))" to just "if (!strcasecmp(v, ...))".
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/push.c | 31 +++++++++++++------------------
builtin/send-pack.c | 27 ++++++++++++---------------
2 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 2e708383c2..58a9273e90 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -526,26 +526,21 @@ static int git_push_config(const char *k, const char *v,
*flags |= TRANSPORT_PUSH_AUTO_UPSTREAM;
return 0;
} else if (!strcmp(k, "push.gpgsign")) {
- const char *value;
- if (!git_config_get_value("push.gpgsign", &value)) {
- switch (git_parse_maybe_bool(value)) {
- case 0:
- set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER);
- break;
- case 1:
- set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS);
- break;
- default:
- if (value && !strcasecmp(value, "if-asked"))
- set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED);
- else
- return error(_("invalid value for '%s'"), k);
- }
+ switch (git_parse_maybe_bool(v)) {
+ case 0:
+ set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER);
+ break;
+ case 1:
+ set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS);
+ break;
+ default:
+ if (!strcasecmp(v, "if-asked"))
+ set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED);
+ else
+ return error(_("invalid value for '%s'"), k);
}
} else if (!strcmp(k, "push.recursesubmodules")) {
- const char *value;
- if (!git_config_get_value("push.recursesubmodules", &value))
- recurse_submodules = parse_push_recurse_submodules_arg(k, value);
+ recurse_submodules = parse_push_recurse_submodules_arg(k, v);
} else if (!strcmp(k, "submodule.recurse")) {
int val = git_config_bool(k, v) ?
RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index cd6d9e4112..00e6c90477 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -135,21 +135,18 @@ static int send_pack_config(const char *k, const char *v,
const struct config_context *ctx, void *cb)
{
if (!strcmp(k, "push.gpgsign")) {
- const char *value;
- if (!git_config_get_value("push.gpgsign", &value)) {
- switch (git_parse_maybe_bool(value)) {
- case 0:
- args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
- break;
- case 1:
- args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
- break;
- default:
- if (value && !strcasecmp(value, "if-asked"))
- args.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED;
- else
- return error(_("invalid value for '%s'"), k);
- }
+ switch (git_parse_maybe_bool(v)) {
+ case 0:
+ args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
+ break;
+ case 1:
+ args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
+ break;
+ default:
+ if (!strcasecmp(v, "if-asked"))
+ args.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED;
+ else
+ return error(_("invalid value for '%s'"), k);
}
}
return git_default_config(k, v, ctx, cb);
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 6/9] config: use git_config_string() for core.checkRoundTripEncoding
From: Jeff King @ 2023-12-07 7:26 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
Since this code path was recently converted to check for a NULL value,
it now behaves exactly like git_config_string(). We can shorten the code
a bit by using that helper.
Note that git_config_string() takes a const pointer, but our storage
variable is non-const. We're better off making this "const", though,
since the default value points to a string literal (and thus it would be
an error if anybody tried to write to it).
Signed-off-by: Jeff King <peff@peff.net>
---
An observant reader may notice that this means duplicate config like:
[core]
checkRoundTripEncoding = foo
checkRoundTripEncoding = bar
will leak the string for "foo". That is true before this patch, too, and
is true of all callers of git_config_string(). I'm going to punt on that
for now, and look into it as a separate series.
config.c | 8 ++------
convert.h | 2 +-
environment.c | 2 +-
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/config.c b/config.c
index d997c55e33..00a11b5d98 100644
--- a/config.c
+++ b/config.c
@@ -1551,12 +1551,8 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
- if (!strcmp(var, "core.checkroundtripencoding")) {
- if (!value)
- return config_error_nonbool(var);
- check_roundtrip_encoding = xstrdup(value);
- return 0;
- }
+ if (!strcmp(var, "core.checkroundtripencoding"))
+ return git_config_string(&check_roundtrip_encoding, var, value);
if (!strcmp(var, "core.notesref")) {
if (!value)
diff --git a/convert.h b/convert.h
index d925589444..ab8b4fa68d 100644
--- a/convert.h
+++ b/convert.h
@@ -92,7 +92,7 @@ void convert_attrs(struct index_state *istate,
struct conv_attrs *ca, const char *path);
extern enum eol core_eol;
-extern char *check_roundtrip_encoding;
+extern const char *check_roundtrip_encoding;
const char *get_cached_convert_stats_ascii(struct index_state *istate,
const char *path);
const char *get_wt_convert_stats_ascii(const char *path);
diff --git a/environment.c b/environment.c
index 9e37bf58c0..90632a39bc 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,7 @@ const char *excludes_file;
enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
-char *check_roundtrip_encoding = "SHIFT-JIS";
+const char *check_roundtrip_encoding = "SHIFT-JIS";
enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 5/9] diff: give more detailed messages for bogus diff.* config
From: Jeff King @ 2023-12-07 7:25 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
The config callbacks for a few diff.* variables simply return -1 when we
encounter an error. The message you get mentions the offending location,
like:
fatal: bad config variable 'diff.algorithm' in file '.git/config' at line 7
but is vague about "bad" (as it must be, since the message comes from
the generic config code). Most callbacks add their own messages here, so
let's do the same. E.g.:
error: unknown value for config 'diff.algorithm': foo
fatal: bad config variable 'diff.algorithm' in file '.git/config' at line 7
I've written the string in a way that should be reusable for
translators, and matches another similar message in transport.c (there
doesn't yet seem to be a popular generic message to reuse here, so
hopefully this will get the ball rolling).
Note that in the case of diff.algorithm, our parse_algorithm_value()
helper does detect a NULL value string. But it's still worth detecting
it ourselves here, since we can give a more specific error message (and
which is the usual one for unexpected implicit-bool values).
Signed-off-by: Jeff King <peff@peff.net>
---
diff.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/diff.c b/diff.c
index 5b213a4b44..a2def45644 100644
--- a/diff.c
+++ b/diff.c
@@ -445,9 +445,12 @@ int git_diff_ui_config(const char *var, const char *value,
}
if (!strcmp(var, "diff.algorithm")) {
+ if (!value)
+ return config_error_nonbool(var);
diff_algorithm = parse_algorithm_value(value);
if (diff_algorithm < 0)
- return -1;
+ return error(_("unknown value for config '%s': %s"),
+ var, value);
return 0;
}
@@ -486,7 +489,8 @@ int git_diff_basic_config(const char *var, const char *value,
return config_error_nonbool(var);
val = parse_ws_error_highlight(value);
if (val < 0)
- return -1;
+ return error(_("unknown value for config '%s': %s"),
+ var, value);
ws_error_highlight_default = val;
return 0;
}
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 4/9] config: use config_error_nonbool() instead of custom messages
From: Jeff King @ 2023-12-07 7:25 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
A few config callbacks use their own custom messages to report an
unexpected implicit bool like:
[merge "foo"]
driver
These should just use config_error_nonbool(), so the user sees
consistent messages.
Signed-off-by: Jeff King <peff@peff.net>
---
imap-send.c | 2 +-
merge-ll.c | 2 +-
xdiff-interface.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 5b0fe4f95a..9c4df7bbc8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,
server.port = git_config_int(var, val, ctx->kvi);
else if (!strcmp("imap.host", var)) {
if (!val) {
- return error("Missing value for 'imap.host'");
+ return config_error_nonbool(var);
} else {
if (starts_with(val, "imap:"))
val += 5;
diff --git a/merge-ll.c b/merge-ll.c
index 8fcf2d3710..1df58ebaac 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -301,7 +301,7 @@ static int read_merge_config(const char *var, const char *value,
if (!strcmp("driver", key)) {
if (!value)
- return error("%s: lacks value", var);
+ return config_error_nonbool(var);
/*
* merge.<name>.driver specifies the command line:
*
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 05d6475a09..d788689d01 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -314,7 +314,7 @@ int git_xmerge_config(const char *var, const char *value,
{
if (!strcmp(var, "merge.conflictstyle")) {
if (!value)
- return error(_("'%s' is not a boolean"), var);
+ return config_error_nonbool(var);
if (!strcmp(value, "diff3"))
git_xmerge_style = XDL_MERGE_DIFF3;
else if (!strcmp(value, "zdiff3"))
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 3/9] imap-send: don't use git_die_config() inside callback
From: Jeff King @ 2023-12-07 7:24 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
The point of git_die_config() is to let configset users mention the
file/line info for invalid config, like:
if (!git_config_get_int("foo.bar", &value)) {
if (!is_ok(value))
git_die_config("foo.bar");
}
Using it from within a config callback is unnecessary, because we can
simply return an error, at which point the config machinery will mention
the file/line of the offending variable. Worse, using git_die_config()
can actually produce the wrong location when the key is found in
multiple spots. For instance, with config like:
[imap]
host
host = foo
we'll report the line number of the "host = foo" line, but the problem
is on the implicit-bool "host" line.
We can fix it by just returning an error code.
Signed-off-by: Jeff King <peff@peff.net>
---
imap-send.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/imap-send.c b/imap-send.c
index 996651e4f8..5b0fe4f95a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,
server.port = git_config_int(var, val, ctx->kvi);
else if (!strcmp("imap.host", var)) {
if (!val) {
- git_die_config("imap.host", "Missing value for 'imap.host'");
+ return error("Missing value for 'imap.host'");
} else {
if (starts_with(val, "imap:"))
val += 5;
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 2/9] git_xmerge_config(): prefer error() to die()
From: Jeff King @ 2023-12-07 7:24 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
When parsing merge config, a few code paths die on error. It's
preferable for us to call error() here, because the resulting error
message from the config parsing code contains much more detail.
For example, before:
fatal: unknown style 'bogus' given for 'merge.conflictstyle'
and after:
error: unknown style 'bogus' given for 'merge.conflictstyle'
fatal: bad config variable 'merge.conflictstyle' in file '.git/config' at line 7
Since we're touching these lines, I also marked them for translation.
There's no reason they shouldn't behave like most other config-parsing
errors.
Signed-off-by: Jeff King <peff@peff.net>
---
Before anyone mentions config_error_nonbool(), yes, the first hunk here
gets simplified to that in a later patch.
xdiff-interface.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/xdiff-interface.c b/xdiff-interface.c
index adcea109fa..05d6475a09 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -1,4 +1,5 @@
#include "git-compat-util.h"
+#include "gettext.h"
#include "config.h"
#include "hex.h"
#include "object-store-ll.h"
@@ -313,7 +314,7 @@ int git_xmerge_config(const char *var, const char *value,
{
if (!strcmp(var, "merge.conflictstyle")) {
if (!value)
- die("'%s' is not a boolean", var);
+ return error(_("'%s' is not a boolean"), var);
if (!strcmp(value, "diff3"))
git_xmerge_style = XDL_MERGE_DIFF3;
else if (!strcmp(value, "zdiff3"))
@@ -325,8 +326,8 @@ int git_xmerge_config(const char *var, const char *value,
* git-completion.bash when you add new merge config
*/
else
- die("unknown style '%s' given for '%s'",
- value, var);
+ return error(_("unknown style '%s' given for '%s'"),
+ value, var);
return 0;
}
return git_default_config(var, value, ctx, cb);
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 1/9] config: reject bogus values for core.checkstat
From: Jeff King @ 2023-12-07 7:24 UTC (permalink / raw)
To: git
In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net>
If you feed nonsense config like:
git -c core.checkstat=foobar status
we'll silently ignore the unknown value, rather than reporting an error.
This goes all the way back to c08e4d5b5c (Enable minimal stat checking,
2013-01-22).
Detecting and complaining now is technically a backwards-incompatible
change, but I don't think anybody has any reason to use an invalid value
here. There are no historical values we'd want to allow for backwards
compatibility or anything like that. We are better off loudly telling
the user that their config may not be doing what they expect.
Signed-off-by: Jeff King <peff@peff.net>
---
config.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/config.c b/config.c
index 18085c7e38..d997c55e33 100644
--- a/config.c
+++ b/config.c
@@ -1392,6 +1392,9 @@ static int git_default_core_config(const char *var, const char *value,
check_stat = 1;
else if (!strcasecmp(value, "minimal"))
check_stat = 0;
+ else
+ return error(_("invalid value for '%s': '%s'"),
+ var, value);
}
if (!strcmp(var, "core.quotepath")) {
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 0/9] bonus config cleanups
From: Jeff King @ 2023-12-07 7:23 UTC (permalink / raw)
To: git
While looking carefully at various config callbacks for the series at:
https://lore.kernel.org/git/20231207071030.GA1275835@coredump.intra.peff.net/
I noticed a bunch of other small bugs/cleanups. I split these into their
own series here, which should be applied on top (it could go straight to
"master", but there is a small conflict in patch 6, as the option it
touches was fixed in the other series). I'm happy to prepare it as an
independent series if we prefer.
[1/9]: config: reject bogus values for core.checkstat
[2/9]: git_xmerge_config(): prefer error() to die()
[3/9]: imap-send: don't use git_die_config() inside callback
[4/9]: config: use config_error_nonbool() instead of custom messages
[5/9]: diff: give more detailed messages for bogus diff.* config
[6/9]: config: use git_config_string() for core.checkRoundTripEncoding
[7/9]: push: drop confusing configset/callback redundancy
[8/9]: gpg-interface: drop pointless config_error_nonbool() checks
[9/9]: sequencer: simplify away extra git_config_string() call
builtin/push.c | 31 +++++++++++++------------------
builtin/send-pack.c | 27 ++++++++++++---------------
config.c | 11 +++++------
convert.h | 2 +-
diff.c | 8 ++++++--
environment.c | 2 +-
gpg-interface.c | 15 +++------------
imap-send.c | 2 +-
merge-ll.c | 2 +-
sequencer.c | 21 ++++++++-------------
xdiff-interface.c | 7 ++++---
11 files changed, 55 insertions(+), 73 deletions(-)
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] setup: extract function to create the refdb
From: Patrick Steinhardt @ 2023-12-07 7:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZSZztJUF9nmSzGdOW0oWBRUp2sw8QyuZO_q06cNymad3Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2242 bytes --]
On Wed, Dec 06, 2023 at 10:10:37PM +0100, Karthik Nayak wrote:
> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt <ps@pks.im> wrote:
> > +static void create_reference_database(const char *initial_branch, int quiet)
> > +{
> > + struct strbuf err = STRBUF_INIT;
> > + int reinit = is_reinit();
> > +
> > + /*
> > + * We need to create a "refs" dir in any case so that older
> > + * versions of git can tell that this is a repository.
> > + */
>
> How does this work though, even if an earlier version of git can tell
> that this is a repository, it still won't be able to read the reftable
> backend. In that sense, what do we achieve here?
This is a good question, and there is related ongoing discussion about
this topic in the thread starting at [1]. There are a few benefits to
letting clients discover such repos even if they don't understand the
new reference backend format:
- They know to stop walking up the parent-directory chain. Otherwise a
client might end up detecting a Git repository in the parent dir.
- The user gets a proper error message why the repository cannot be
accessed. Instead of failing to detect the repository altogether we
instead say that we don't understand the "extensions.refFormat"
extension.
Maybe there are other cases I can't think of right now.
> > + safe_create_dir(git_path("refs"), 1);
> > + adjust_shared_perm(git_path("refs"));
> > +
>
> Not related to your commit per se, but we ignore the return value
> here, shouldn't we die in this case?
While the end result wouldn't be quite what the user asks for, the only
negative consequence is that the repository is inaccessible to others. I
think this failure mode is comparatively benign -- if it were the other
way round and we'd over-share the repository it would more severe.
So while I don't think that dying makes much sense here, I could
certainly see us adding a warning so that the user at least knows that
something went wrong. I'd rather want to keep this out of the current
patch series, but could certainly see such a warning added in a follow
up patch series.
Patrick
[1]: <ZWcOvjGPVS_CMUAk@tanuki>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/7] builtin/clone: fix bundle URIs with mismatching object formats
From: Patrick Steinhardt @ 2023-12-07 7:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZQc=7Z3w9JAdzS23P=c=KSYZJR6gJSLOHdU-d92Y3kJ5A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 845 bytes --]
On Wed, Dec 06, 2023 at 10:13:43PM +0100, Karthik Nayak wrote:
> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt <ps@pks.im> wrote:
> > The first Git step where we expect the repository to be fully initalized
> > is when we fetch bundles via bundle URIs. Funny enough, the comments
> > there also state that "the_repository must match the cloned repo", which
> > is indeed not necessarily the case for the hash algorithm right now. So
> > in practice it is the right thing to detect the remote's object format
> > before downloading bundle URIs anyway, and not doing so causes clones
> > with bundle URIS to fail when the local default object format does not
> > match the remote repository's format.
> >
>
> Nit: s/URIS/URIs
Thanks, fixed locally. Will wait with v2 though until there are more
review comments.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 7/7] fsck: handle NULL value when parsing message config
From: Jeff King @ 2023-12-07 7:11 UTC (permalink / raw)
To: git; +Cc: Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071030.GA1275835@coredump.intra.peff.net>
When parsing fsck.*, receive.fsck.*, or fetch.fsck.*, we don't check for
an implicit bool. So any of:
[fsck]
badTree
[receive "fsck"]
badTree
[fetch "fsck"]
badTree
will cause us to segfault. We can fix it with config_error_nonbool() in
the usual way, but we have to make a few more changes to get good error
messages. The problem is that all three spots do:
if (skip_prefix(var, "fsck.", &var))
to match and parse the actual message id. But that means that "var" now
just says "badTree" instead of "receive.fsck.badTree", making the
resulting message confusing. We can fix that by storing the parsed
message id in its own separate variable.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/receive-pack.c | 11 +++++++----
fetch-pack.c | 12 ++++++++----
fsck.c | 8 ++++++--
3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8c4f0cb90a..ccf9738bce 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -142,6 +142,7 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
static int receive_pack_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
{
+ const char *msg_id;
int status = parse_hide_refs_config(var, value, "receive", &hidden_refs);
if (status)
@@ -178,12 +179,14 @@ static int receive_pack_config(const char *var, const char *value,
return 0;
}
- if (skip_prefix(var, "receive.fsck.", &var)) {
- if (is_valid_msg_type(var, value))
+ if (skip_prefix(var, "receive.fsck.", &msg_id)) {
+ if (!value)
+ return config_error_nonbool(var);
+ if (is_valid_msg_type(msg_id, value))
strbuf_addf(&fsck_msg_types, "%c%s=%s",
- fsck_msg_types.len ? ',' : '=', var, value);
+ fsck_msg_types.len ? ',' : '=', msg_id, value);
else
- warning("skipping unknown msg id '%s'", var);
+ warning("skipping unknown msg id '%s'", msg_id);
return 0;
}
diff --git a/fetch-pack.c b/fetch-pack.c
index 26999e3b65..31a72d43de 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1862,6 +1862,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
static int fetch_pack_config_cb(const char *var, const char *value,
const struct config_context *ctx, void *cb)
{
+ const char *msg_id;
+
if (strcmp(var, "fetch.fsck.skiplist") == 0) {
const char *path;
@@ -1873,12 +1875,14 @@ static int fetch_pack_config_cb(const char *var, const char *value,
return 0;
}
- if (skip_prefix(var, "fetch.fsck.", &var)) {
- if (is_valid_msg_type(var, value))
+ if (skip_prefix(var, "fetch.fsck.", &msg_id)) {
+ if (!value)
+ return config_error_nonbool(var);
+ if (is_valid_msg_type(msg_id, value))
strbuf_addf(&fsck_msg_types, "%c%s=%s",
- fsck_msg_types.len ? ',' : '=', var, value);
+ fsck_msg_types.len ? ',' : '=', msg_id, value);
else
- warning("Skipping unknown msg id '%s'", var);
+ warning("Skipping unknown msg id '%s'", msg_id);
return 0;
}
diff --git a/fsck.c b/fsck.c
index 6a0bbc5087..b624083a13 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1403,6 +1403,8 @@ int git_fsck_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
{
struct fsck_options *options = cb;
+ const char *msg_id;
+
if (strcmp(var, "fsck.skiplist") == 0) {
const char *path;
struct strbuf sb = STRBUF_INIT;
@@ -1416,8 +1418,10 @@ int git_fsck_config(const char *var, const char *value,
return 0;
}
- if (skip_prefix(var, "fsck.", &var)) {
- fsck_set_msg_type(options, var, value);
+ if (skip_prefix(var, "fsck.", &msg_id)) {
+ if (!value)
+ return config_error_nonbool(var);
+ fsck_set_msg_type(options, msg_id, value);
return 0;
}
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 6/7] trailer: handle NULL value when parsing trailer-specific config
From: Jeff King @ 2023-12-07 7:11 UTC (permalink / raw)
To: git; +Cc: Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071030.GA1275835@coredump.intra.peff.net>
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:
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 5/7] submodule: handle NULL value when parsing submodule.*.branch
From: Jeff King @ 2023-12-07 7:11 UTC (permalink / raw)
To: git; +Cc: Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071030.GA1275835@coredump.intra.peff.net>
We record the submodule branch config value as a string, so config that
uses an implicit bool like:
[submodule "foo"]
branch
will cause us to segfault. Note that unlike most other config-parsing
bugs of this class, this can be triggered by parsing a bogus .gitmodules
file (which we might do after cloning a malicious repository).
I don't think the security implications are important, though. It's
always a strict NULL dereference, not an out-of-bounds read or write. So
we should reliably kill the process. That may be annoying, but the
impact is limited to the attacker preventing the victim from
successfully using "git clone --recurse-submodules", etc, on the
malicious repo.
The "branch" entry is the only one with this problem; other strings like
"path" and "url" already check for NULL.
Signed-off-by: Jeff King <peff@peff.net>
---
submodule-config.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/submodule-config.c b/submodule-config.c
index 6a48fd12f6..f4dd482abc 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value,
submodule->recommend_shallow =
git_config_bool(var, value);
} else if (!strcmp(item.buf, "branch")) {
- if (!me->overwrite && submodule->branch)
+ if (!value)
+ ret = config_error_nonbool(var);
+ else if (!me->overwrite && submodule->branch)
warn_multiple_config(me->treeish_name, submodule->name,
"branch");
else {
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 4/7] help: handle NULL value for alias.* config
From: Jeff King @ 2023-12-07 7:11 UTC (permalink / raw)
To: git; +Cc: Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071030.GA1275835@coredump.intra.peff.net>
When showing all config with "git help --all", we print the list of
defined aliases. But our config callback to do so does not check for a
NULL value, meaning a config block like:
[alias]
foo
will cause us to segfault. We should detect and complain about this in
the usual way.
Since this command is purely informational (and we aren't trying to run
the alias), we could perhaps just generate a warning and continue. But
this sort of misconfiguration should be pretty rare, and the error
message we will produce points directly to the line of config that needs
to be fixed. So just generating the usual error should be OK.
Signed-off-by: Jeff King <peff@peff.net>
---
help.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/help.c b/help.c
index 6d2ebfbd2a..2dbe57b413 100644
--- a/help.c
+++ b/help.c
@@ -464,8 +464,11 @@ static int get_alias(const char *var, const char *value,
{
struct string_list *list = data;
- if (skip_prefix(var, "alias.", &var))
+ if (skip_prefix(var, "alias.", &var)) {
+ if (!value)
+ return config_error_nonbool(var);
string_list_append(list, var)->util = xstrdup(value);
+ }
return 0;
}
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 3/7] trace2: handle NULL values in tr2_sysenv config callback
From: Jeff King @ 2023-12-07 7:11 UTC (permalink / raw)
To: git; +Cc: Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071030.GA1275835@coredump.intra.peff.net>
If you have config with an implicit bool like:
[trace2]
envvars
we'll segfault, as we unconditionally try to xstrdup() the value. We
should instead detect and complain, as a boolean value has no meaning
here. The same is true for every variable in tr2_sysenv_settings (and
this patch covers them all, as we check them in a loop).
Signed-off-by: Jeff King <peff@peff.net>
---
trace2/tr2_sysenv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
index d3ecac2772..048cdd5438 100644
--- a/trace2/tr2_sysenv.c
+++ b/trace2/tr2_sysenv.c
@@ -68,6 +68,8 @@ static int tr2_sysenv_cb(const char *key, const char *value,
for (k = 0; k < ARRAY_SIZE(tr2_sysenv_settings); k++) {
if (!strcmp(key, tr2_sysenv_settings[k].git_config_name)) {
+ if (!value)
+ return config_error_nonbool(key);
free(tr2_sysenv_settings[k].value);
tr2_sysenv_settings[k].value = xstrdup(value);
return 0;
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 2/7] setup: handle NULL value when parsing extensions
From: Jeff King @ 2023-12-07 7:11 UTC (permalink / raw)
To: git; +Cc: Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071030.GA1275835@coredump.intra.peff.net>
The "partialclone" extension config records a string, and hence it is an
error to have an implicit bool like:
[extensions]
partialclone
in your config. We should recognize and reject this, rather than
segfaulting (which is the current behavior). Note that it's OK to use
config_error_nonbool() here, even though the return value is an enum. We
explicitly document EXTENSION_ERROR as -1 for compatibility with
error(), etc.
This is the only extension value that has this problem. Most of the
others are bools that interpret this value naturally. The exception is
extensions.objectformat, which does correctly check for NULL.
Signed-off-by: Jeff King <peff@peff.net>
---
setup.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/setup.c b/setup.c
index fc592dc6dd..50131be7cc 100644
--- a/setup.c
+++ b/setup.c
@@ -559,6 +559,8 @@ static enum extension_result handle_extension_v0(const char *var,
data->precious_objects = git_config_bool(var, value);
return EXTENSION_OK;
} else if (!strcmp(ext, "partialclone")) {
+ if (!value)
+ return config_error_nonbool(var);
data->partial_clone = xstrdup(value);
return EXTENSION_OK;
} else if (!strcmp(ext, "worktreeconfig")) {
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 1/7] config: handle NULL value when parsing non-bools
From: Jeff King @ 2023-12-07 7:11 UTC (permalink / raw)
To: git; +Cc: Carlos Andrés Ramírez Cataño
In-Reply-To: <20231207071030.GA1275835@coredump.intra.peff.net>
When the config parser sees an "implicit" bool like:
[core]
someVariable
it passes NULL to the config callback. Any callback code which expects a
string must check for NULL. This usually happens via helpers like
git_config_string(), etc, but some custom code forgets to do so and will
segfault.
These are all fairly vanilla cases where the solution is just the usual
pattern of:
if (!value)
return config_error_nonbool(var);
though note that in a few cases we have to split initializers like:
int some_var = initializer();
into:
int some_var;
if (!value)
return config_error_nonbool(var);
some_var = initializer();
There are still some broken instances after this patch, which I'll
address on their own in individual patches after this one.
Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/blame.c | 2 ++
builtin/checkout.c | 2 ++
builtin/clone.c | 2 ++
builtin/log.c | 5 ++++-
builtin/pack-objects.c | 6 +++++-
compat/mingw.c | 2 ++
config.c | 8 ++++++++
diff.c | 19 ++++++++++++++++---
mailinfo.c | 2 ++
notes-utils.c | 2 ++
trailer.c | 2 ++
11 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 9c987d6567..2433b7da5c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value,
}
if (!strcmp(var, "blame.coloring")) {
+ if (!value)
+ return config_error_nonbool(var);
if (!strcmp(value, "repeatedLines")) {
coloring_mode |= OUTPUT_COLOR_LINE;
} else if (!strcmp(value, "highlightRecent")) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc15..d5c784854f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value,
struct checkout_opts *opts = cb;
if (!strcmp(var, "diff.ignoresubmodules")) {
+ if (!value)
+ return config_error_nonbool(var);
handle_ignore_submodules_arg(&opts->diff_options, value);
return 0;
}
diff --git a/builtin/clone.c b/builtin/clone.c
index c6357af949..54d9b9976a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -791,6 +791,8 @@ static int git_clone_config(const char *k, const char *v,
const struct config_context *ctx, void *cb)
{
if (!strcmp(k, "clone.defaultremotename")) {
+ if (!v)
+ return config_error_nonbool(k);
free(remote_name);
remote_name = xstrdup(v);
}
diff --git a/builtin/log.c b/builtin/log.c
index ba775d7b5c..3ce41c4856 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -594,8 +594,11 @@ static int git_log_config(const char *var, const char *value,
decoration_style = 0; /* maybe warn? */
return 0;
}
- if (!strcmp(var, "log.diffmerges"))
+ if (!strcmp(var, "log.diffmerges")) {
+ if (!value)
+ return config_error_nonbool(var);
return diff_merges_config(value);
+ }
if (!strcmp(var, "log.showroot")) {
default_show_root = git_config_bool(var, value);
return 0;
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 89a8b5a976..62c540b4db 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v,
return 0;
}
if (!strcmp(k, "uploadpack.blobpackfileuri")) {
- struct configured_exclusion *ex = xmalloc(sizeof(*ex));
+ struct configured_exclusion *ex;
const char *oid_end, *pack_end;
/*
* Stores the pack hash. This is not a true object ID, but is
* of the same form.
*/
struct object_id pack_hash;
+ if (!v)
+ return config_error_nonbool(k);
+
+ ex = xmalloc(sizeof(*ex));
if (parse_oid_hex(v, &ex->e.oid, &oid_end) ||
*oid_end != ' ' ||
parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||
diff --git a/compat/mingw.c b/compat/mingw.c
index ec5280da16..42053c1f65 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -255,6 +255,8 @@ int mingw_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.unsetenvvars")) {
+ if (!value)
+ return config_error_nonbool(var);
free(unset_environment_variables);
unset_environment_variables = xstrdup(value);
return 0;
diff --git a/config.c b/config.c
index b330c7adb4..18085c7e38 100644
--- a/config.c
+++ b/config.c
@@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
if (!strcmp(var, "core.checkstat")) {
+ if (!value)
+ return config_error_nonbool(var);
if (!strcasecmp(value, "default"))
check_stat = 1;
else if (!strcasecmp(value, "minimal"))
@@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.checkroundtripencoding")) {
+ if (!value)
+ return config_error_nonbool(var);
check_roundtrip_encoding = xstrdup(value);
return 0;
}
if (!strcmp(var, "core.notesref")) {
+ if (!value)
+ return config_error_nonbool(var);
notes_ref_name = xstrdup(value);
return 0;
}
@@ -1619,6 +1625,8 @@ static int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.createobject")) {
+ if (!value)
+ return config_error_nonbool(var);
if (!strcmp(value, "rename"))
object_creation_mode = OBJECT_CREATION_USES_RENAMES;
else if (!strcmp(value, "link"))
diff --git a/diff.c b/diff.c
index 2c602df10a..5b213a4b44 100644
--- a/diff.c
+++ b/diff.c
@@ -372,7 +372,10 @@ int git_diff_ui_config(const char *var, const char *value,
return 0;
}
if (!strcmp(var, "diff.colormovedws")) {
- unsigned cm = parse_color_moved_ws(value);
+ unsigned cm;
+ if (!value)
+ return config_error_nonbool(var);
+ cm = parse_color_moved_ws(value);
if (cm & COLOR_MOVED_WS_ERROR)
return -1;
diff_color_moved_ws_default = cm;
@@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value,
if (!strcmp(var, "diff.orderfile"))
return git_config_pathname(&diff_order_file_cfg, var, value);
- if (!strcmp(var, "diff.ignoresubmodules"))
+ if (!strcmp(var, "diff.ignoresubmodules")) {
+ if (!value)
+ return config_error_nonbool(var);
handle_ignore_submodules_arg(&default_diff_options, value);
+ }
if (!strcmp(var, "diff.submodule")) {
+ if (!value)
+ return config_error_nonbool(var);
if (parse_submodule_params(&default_diff_options, value))
warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
value);
@@ -473,7 +481,10 @@ int git_diff_basic_config(const char *var, const char *value,
}
if (!strcmp(var, "diff.wserrorhighlight")) {
- int val = parse_ws_error_highlight(value);
+ int val;
+ if (!value)
+ return config_error_nonbool(var);
+ val = parse_ws_error_highlight(value);
if (val < 0)
return -1;
ws_error_highlight_default = val;
@@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value,
if (!strcmp(var, "diff.dirstat")) {
struct strbuf errmsg = STRBUF_INIT;
+ if (!value)
+ return config_error_nonbool(var);
default_diff_options.dirstat_permille = diff_dirstat_permille_default;
if (parse_dirstat_params(&default_diff_options, value, &errmsg))
warning(_("Found errors in 'diff.dirstat' config variable:\n%s"),
diff --git a/mailinfo.c b/mailinfo.c
index a07d2da16d..093bed5d8f 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1253,6 +1253,8 @@ static int git_mailinfo_config(const char *var, const char *value,
return 0;
}
if (!strcmp(var, "mailinfo.quotedcr")) {
+ if (!value)
+ return config_error_nonbool(var);
if (mailinfo_parse_quoted_cr_action(value, &mi->quoted_cr) != 0)
return error(_("bad action '%s' for '%s'"), value, var);
return 0;
diff --git a/notes-utils.c b/notes-utils.c
index 97c031c26e..01f4f5b424 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v,
}
return 0;
} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
+ if (!v)
+ return config_error_nonbool(k);
/* note that a refs/ prefix is implied in the
* underlying for_each_glob_ref */
if (starts_with(v, "refs/notes/"))
diff --git a/trailer.c b/trailer.c
index b6de5d9cb2..b0e2ec224a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "separators")) {
+ if (!value)
+ return config_error_nonbool(conf_key);
separators = xstrdup(value);
}
}
--
2.43.0.664.ga12c899002
^ permalink raw reply related
* [PATCH 0/7] fix segfaults with implicit-bool config
From: Jeff King @ 2023-12-07 7:10 UTC (permalink / raw)
To: git; +Cc: Carlos Andrés Ramírez Cataño
Carlos reported to the security list a case where you can cause Git
to segfault by using an implicit bool like:
[core]
someVariable
when the parsing side for core.someVariable does not correctly check a
NULL "value" string. This is mostly harmless, as anybody who can feed
arbitrary config can already execute arbitrary code. There is one case
of this when parsing .gitmodules (which we don't trust), but even there
I don't think the security implications are that interesting. A
malicious repo can get "clone --recurse-submodules" to segfault, but
always with a strict NULL dereference, not any kind of controllable
pointer. See patch 5 for more details.
I audited the whole code base for instances of the problem. It was
fairly manual, so it's possible I missed a spot, but I think this should
cover everything.
The first patch has vanilla cases, and the rest are instances where I
thought it was worth calling out specific details.
[1/7]: config: handle NULL value when parsing non-bools
[2/7]: setup: handle NULL value when parsing extensions
[3/7]: trace2: handle NULL values in tr2_sysenv config callback
[4/7]: help: handle NULL value for alias.* config
[5/7]: submodule: handle NULL value when parsing submodule.*.branch
[6/7]: trailer: handle NULL value when parsing trailer-specific config
[7/7]: fsck: handle NULL value when parsing message config
builtin/blame.c | 2 ++
builtin/checkout.c | 2 ++
builtin/clone.c | 2 ++
builtin/log.c | 5 ++++-
builtin/pack-objects.c | 6 +++++-
builtin/receive-pack.c | 11 +++++++----
compat/mingw.c | 2 ++
config.c | 8 ++++++++
diff.c | 19 ++++++++++++++++---
fetch-pack.c | 12 ++++++++----
fsck.c | 8 ++++++--
help.c | 5 ++++-
mailinfo.c | 2 ++
notes-utils.c | 2 ++
setup.c | 2 ++
submodule-config.c | 4 +++-
trace2/tr2_sysenv.c | 2 ++
trailer.c | 8 ++++++++
18 files changed, 85 insertions(+), 17 deletions(-)
^ permalink raw reply
* Re: [PATCH 4/7] revision, rev-parse: factorize incompatibility messages about --exclude-hidden
From: Patrick Steinhardt @ 2023-12-07 7:10 UTC (permalink / raw)
To: Taylor Blau; +Cc: René Scharfe, git
In-Reply-To: <ZXDKjdOiIdHipaKy@nand.local>
[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]
On Wed, Dec 06, 2023 at 02:25:01PM -0500, Taylor Blau wrote:
> On Wed, Dec 06, 2023 at 06:07:29PM +0100, René Scharfe wrote:
> > > It's not perfect
> > > of course, but would at least ensure that we can easily convert things
> > > over time without having to duplicate the exact message everywhere.
> >
> > Maybe the simplest option would be to use a macro, e.g.
> >
> > #define INCOMPATIBLE_OPTIONS_MESSAGE \
> > _("options '%s' and '%s' cannot be used together")
> >
> > It could be used with both error() and die(), and the compiler would
> > still ensure that two strings are passed along with it, but I don't know
> > how to encode that requirement in the macro name somehow to make it
> > self-documenting. Perhaps by getting the number two in there?
>
> I think that this is a great idea. It nicely solves Patrick's concern
> that we have to duplicate this message ID everywhere, and equally solves
> yours by calling error() inline instead of having to pass down the
> option values.
>
> I think that including a number in the macro name would be helpful here.
Does our i18n tooling know how to extract such messages defined in
macros? I have to admit I don't really know how it works under the hood.
But if it does work then this looks like a good solution to me.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] setup: recognize bare repositories with packed-refs
From: Patrick Steinhardt @ 2023-12-07 7:01 UTC (permalink / raw)
To: Jeff King; +Cc: Adam Majer, git
In-Reply-To: <20231206201048.GE103708@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 5150 bytes --]
On Wed, Dec 06, 2023 at 03:10:48PM -0500, Jeff King wrote:
> On Wed, Nov 29, 2023 at 11:13:18AM +0100, Patrick Steinhardt wrote:
>
> > As I'm currently working on the reftable backend this thought has also
> > crossed my mind. The reftable backend doesn't only create "refs/", but
> > it also creates "HEAD" with contents "ref: refs/heads/.invalid" so that
> > Git commands recognize the Git directory properly. Longer-term I would
> > really love to see us doing a better job of detecting Git repositories
> > so that we don't have to carry this legacy baggage around.
> >
> > I can see different ways for how to do this:
> >
> > - Either we iterate through all known reference backends, asking
> > each of them whether they recognize the directory as something
> > they understand.
> >
> > - Or we start parsing the gitconfig of the repository so that we can
> > learn about which reference backend to expect, and then ask that
> > specific backend whether it thinks that the directory indeed looks
> > like something it can handle.
> >
> > I'd personally prefer the latter, but I'm not sure whether we really
> > want to try and parse any file that happens to be called "config".
>
> We do eventually parse the config file to pick up repositoryFormatVersion.
> But there's sort of a chicken-and-egg here where we only do so after
> gaining some confidence that it's a repo directory. :)
>
> I actually think the "ask each backend if it looks plausible" is
> reasonable, at least for an implementation that knows about all
> backends. Though what gives me pause is how older versions of Git will
> behave with a new-format repository that does not have a "refs"
> directory.
>
> There are really two compatibility checks. In is_git_directory(), we
> want to say "is this a repo or not". And then later we parse the config,
> make sure the repository format is OK, and that we support all
> extensions. So right now, an older version of Git that encounters a
> reftable-formatted repo (that has a vestigial "refs/" directory) says
> "ah, that is a repo, but I don't understand it" (the latter because
> presumably the repo version/extensions in .git/config are values it
> doesn't know about). But if we get rid of "refs/", then older versions
> of Git will stop even considering it as a repo at all, and will keep
> searching up to the ceiling directory. So either:
>
> 1. They'll find nothing, and you'll get "you're not in a git repo",
> rather than "you're in a git repo, but I don't understand it".
> Which is slightly worse.
>
> 2. They'll find some _other_ containing repo. Which could be quite
> confusing.
>
> So forgetting at all about how we structure the code, it seems to me
> that the problem is not new code, but all of the existing code which
> looks for access("refs", X_OK).
True. The question is of course how much value there is in an old tool
to be able to discover a new repository that it wouldn't be able to read
in the first place due to it not understanding the reference format. So
I'd very much like to see that eventually, we're able to get rid of
"legacy" cruft that doesn't serve any purpose anymore.
The question is whether we can do a better job of this going forward so
that at least we don't have to pose the same question in the future.
Right now, we'll face the same problem whenever any part of the current
on-disk repository data structures changes.
I wonder whether it would make sense to introduce something like a
filesystem-level hint, e.g. in the form of a new ".is-git-repository"
file. If Git discovers that file then it assumes the directory to be a
Git repository -- and everything else is set up by parsing the config
and thus the repository's configured format.
> I dunno. Maybe that is being too paranoid about backwards compatibility.
> People will have to turn on reftable manually, at least for a while, and
> would hopefully know what they are signing up for, and that old versions
> might not work as well. And by the time a new format becomes the
> default, it's possible that those older versions would have become quite
> rare.
>
> > Just throwing this out there, but we could use this as an excuse to
> > introduce "extensions.refFormat". If it's explicitly configured to be
> > "reffiles" then we accept repositories even if they don't have the
> > "refs/" directory or a "packed-refs" file. This would still require work
> > in alternative implementations of Git, but this work will need to happen
> > anyway when the reftable backend lands.
> >
> > I'd personally love for this extension to be introduced before I'm
> > sending the reftable backend upstream so that we can have discussions
> > around it beforehand.
>
> We already have an extension config option to specify that we're using
> reftable, don't we? But anything in config has the same chicken-and-egg
> problems as above, I think.
Not yet, no. I plan to submit the new "extensions.refFormat" extension
soonish though, probably next week.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox