From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Jeff Hostetler <jeffhost@microsoft.com>
Subject: [PATCH] upload-pack: fix broken if/else chain in config callback
Date: Wed, 24 Oct 2018 03:27:52 -0400 [thread overview]
Message-ID: <20181024072752.GA29717@sigill.intra.peff.net> (raw)
The upload_pack_config() callback uses an if/else chain
like:
if (!strcmp(var, "a"))
...
else if (!strcmp(var, "b"))
...
etc
This works as long as the conditions are mutually exclusive,
but one of them is not. 20b20a22f8 (upload-pack: provide a
hook for running pack-objects, 2016-05-18) added:
else if (current_config_scope() != CONFIG_SCOPE_REPO) {
... check some more options ...
}
That was fine in that commit, because it came at the end of
the chain. But later, 10ac85c785 (upload-pack: add object
filtering for partial clone, 2017-12-08) did this:
else if (current_config_scope() != CONFIG_SCOPE_REPO) {
... check some more options ...
} else if (!strcmp("uploadpack.allowfilter", var))
...
We'd always check the scope condition first, meaning we'd
_only_ respect allowfilter when it's in the repo config. You
can see this with:
git -c uploadpack.allowfilter=true upload-pack . | head -1
which will not advertise the filter capability (but will
after this patch). We never noticed because:
- our tests always set it in the repo config
- in protocol v2, we use a different code path that
actually calls repo_config_get_bool() separately, so
that _does_ work. Real-world people experimenting with
this may be using v2.
The more recent uploadpack.allowrefinwant option is in the
same boat.
There are a few possible fixes:
1. Bump the scope conditional back to the bottom of the
chain. But that just means somebody else is likely to
make the same mistake later.
2. Make the conditional more like the others. I.e.:
else if (!current_config_scope() != CONFIG_SCOPE_REPO &&
!strcmp(var, "uploadpack.notallowedinrepo"))
This works, but the idea of the original structure was
that we may grow multiple sensitive options like this.
3. Pull it out of the chain entirely. The chain mostly
serves to avoid extra strcmp() calls after we've found
a match. But it's not worth caring about those. In the
worst case, when there isn't a match, we're already
hitting every strcmp (and this happens regularly for
stuff like "core.bare", etc).
This patch does (3).
Signed-off-by: Jeff King <peff@peff.net>
---
Phew. That was a lot of explanation for a small patch, but
this was sufficiently subtle I thought it worth it. And
also, I was really surprised the bug managed to exist for
this long without anybody noticing.
upload-pack.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 540778d1dd..489c18e222 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1028,14 +1028,17 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
keepalive = git_config_int(var, value);
if (!keepalive)
keepalive = -1;
- } else if (current_config_scope() != CONFIG_SCOPE_REPO) {
- if (!strcmp("uploadpack.packobjectshook", var))
- return git_config_string(&pack_objects_hook, var, value);
} else if (!strcmp("uploadpack.allowfilter", var)) {
allow_filter = git_config_bool(var, value);
} else if (!strcmp("uploadpack.allowrefinwant", var)) {
allow_ref_in_want = git_config_bool(var, value);
}
+
+ if (current_config_scope() != CONFIG_SCOPE_REPO) {
+ if (!strcmp("uploadpack.packobjectshook", var))
+ return git_config_string(&pack_objects_hook, var, value);
+ }
+
return parse_hide_refs_config(var, value, "uploadpack");
}
--
2.19.1.1094.gd480080bf6
next reply other threads:[~2018-10-24 7:27 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-24 7:27 Jeff King [this message]
2018-10-24 8:50 ` [PATCH] upload-pack: fix broken if/else chain in config callback Johannes Schindelin
2018-10-25 21:59 ` Josh Steadmon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181024072752.GA29717@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=jeffhost@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.