path: root/builtin/receive-pack.c
diff options
authorÆvar Arnfjörð Bjarmason <>2019-02-20 00:00:33 (GMT)
committerJunio C Hamano <>2019-02-20 23:02:12 (GMT)
commit9903623761e6882a1c885ca4245dd6951052ecc3 (patch)
tree46fe33be9120bd2c04c16ad244d56ca70c5a3424 /builtin/receive-pack.c
parent268fbcd172cdb306e8a3e7143cc16677c963d6cd (diff)
receive-pack: fix use-after-free bug
The resolve_ref_unsafe() function can, and sometimes will in the case of this codepath, return the char * passed to it to the caller. In this case we construct a strbuf, free it, and then continue using the dst_name after that free(). The code being fixed dates back to da3efdb17b ("receive-pack: detect aliased updates which can occur with symrefs", 2010-04-19). When it was originally added it didn't have this bug, it was introduced when it was subsequently modified to use strbuf in 6b01ecfe22 ("ref namespaces: Support remote repositories via upload-pack and receive-pack", 2011-07-08). This is theoretically a security issue, the C standard makes no guarantees that a value you use after free() hasn't been poked at or changed by something else on the system, but in practice modern OSs will have mapped the relevant page to this process, so nothing else would have used it. We do no further allocations between the free() and use-after-free, so we ourselves didn't corrupt or change the value. Jeff investigated that and found: "It probably would be an issue if the allocation were larger. glibc at least will use mmap()/munmap() after some cutoff[1], in which case we'd get a segfault from hitting the unmapped page. But for small allocations, it just bumps brk() and the memory is still available for further allocations after free(). [...] If you had a sufficiently large refname you might be able to trigger the bug [...]. I tried to push such a ref. I had to manually make a packed-refs file with the long name to avoid filesystem limits (though probably you could have a long a/b/c/ name on ext4). But the result can't actually be pushed, because it all has to fit into a 64k pkt-line as part of the push protocol.". An a alternative and more succinct way of implementing this would have been to do the strbuf_release() at the end of check_aliased_update() and use "goto out" instead of the early "return" statements. Hopefully this approach of using a helper instead makes it easier to follow. 1. Jeff: "Weirdly, the mmap() cutoff on my glibc system is 135168 bytes. Which is...2^17 + 2^12? 33 pages? I'm sure there's a good reason for that, but I didn't dig into it." Reported-by: 王健强 <> Helped-by: Jeff King <> Signed-off-by: Ævar Arnfjörð Bjarmason <> Signed-off-by: Junio C Hamano <>
Diffstat (limited to 'builtin/receive-pack.c')
1 files changed, 15 insertions, 8 deletions
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 68d36e0..3b133ce 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1204,17 +1204,12 @@ static void run_update_post_hook(struct command *commands)
-static void check_aliased_update(struct command *cmd, struct string_list *list)
+static void check_aliased_update_internal(struct command *cmd,
+ struct string_list *list,
+ const char *dst_name, int flag)
- struct strbuf buf = STRBUF_INIT;
- const char *dst_name;
struct string_list_item *item;
struct command *dst_cmd;
- int flag;
- strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);
- dst_name = resolve_ref_unsafe(buf.buf, 0, NULL, &flag);
- strbuf_release(&buf);
if (!(flag & REF_ISSYMREF))
@@ -1253,6 +1248,18 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
"inconsistent aliased update";
+static void check_aliased_update(struct command *cmd, struct string_list *list)
+ struct strbuf buf = STRBUF_INIT;
+ const char *dst_name;
+ int flag;
+ strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);
+ dst_name = resolve_ref_unsafe(buf.buf, 0, NULL, &flag);
+ check_aliased_update_internal(cmd, list, dst_name, flag);
+ strbuf_release(&buf);
static void check_aliased_updates(struct command *commands)
struct command *cmd;