From 5ff4b920ebeadfa640f1b7d5929f58bb180a0519 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 9 Apr 2020 17:03:45 -0700 Subject: [PATCH 1/2] sha1-name: do not assume that the ref store is initialized MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit c931ba4e (sha1-name.c: remove the_repo from handle_one_ref(), 2019-04-16) replaced the use of for_each_ref() helper, which works with the main ref store of the default repository instance, with refs_for_each_ref(), which can work on any ref store instance, by assuming that the repository instance the function is given has its ref store already initialized. But it is possible that nobody has initialized it, in which case, the code ends up dereferencing a NULL pointer. Reported-by: Érico Rolim Signed-off-by: Junio C Hamano --- sha1-name.c | 2 +- t/t4208-log-magic-pathspec.sh | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/sha1-name.c b/sha1-name.c index d9050776dd..3aba62938f 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1771,7 +1771,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo, cb.repo = repo; cb.list = &list; - refs_for_each_ref(repo->refs, handle_one_ref, &cb); + refs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb); refs_head_ref(repo->refs, handle_one_ref, &cb); commit_list_sort_by_date(&list); return get_oid_oneline(repo, name + 2, oid, list); diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 4c8f3b8e1b..6cdbe4747a 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -55,6 +55,10 @@ test_expect_success '"git log -- :/a" should not be ambiguous' ' git log -- :/a ' +test_expect_success '"git log :/any/path/" should not segfault' ' + test_must_fail git log :/any/path/ +' + # This differs from the ":/a" check above in that :/in looks like a pathspec, # but doesn't match an actual file. test_expect_success '"git log :/in" should not be ambiguous' ' From 022046107186746913ef43a33f4b6862817bf6da Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Apr 2020 23:04:11 -0400 Subject: [PATCH 2/2] repository: mark the "refs" pointer as private The "refs" pointer in a struct repository starts life as NULL, but then is lazily initialized when it is accessed via get_main_ref_store(). However, it's easy for calling code to forget this and access it directly, leading to code which works _some_ of the time, but fails if it is called before anybody else accesses the refs. This was the cause of the bug fixed by 5ff4b920eb (sha1-name: do not assume that the ref store is initialized, 2020-04-09). In order to prevent similar bugs, let's more clearly mark the "refs" field as private. In addition to helping future code, the name change will help us audit any existing direct uses. Besides get_main_ref_store() itself, it turns out there is only one. But we know it's OK as it is on the line directly after the fix from 5ff4b920eb, which will have initialized the pointer. However it's still a good idea for it to model the proper use of the accessing function, so we'll convert it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 8 ++++---- repository.h | 8 ++++++-- sha1-name.c | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/refs.c b/refs.c index 92d1f6dbdd..4cc32e7aad 100644 --- a/refs.c +++ b/refs.c @@ -1774,14 +1774,14 @@ static struct ref_store *ref_store_init(const char *gitdir, struct ref_store *get_main_ref_store(struct repository *r) { - if (r->refs) - return r->refs; + if (r->refs_private) + return r->refs_private; if (!r->gitdir) BUG("attempting to get main_ref_store outside of repository"); - r->refs = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS); - return r->refs; + r->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS); + return r->refs_private; } /* diff --git a/repository.h b/repository.h index 4fb6a5885f..4ecc201f89 100644 --- a/repository.h +++ b/repository.h @@ -39,8 +39,12 @@ struct repository { */ struct parsed_object_pool *parsed_objects; - /* The store in which the refs are held. */ - struct ref_store *refs; + /* + * The store in which the refs are held. This should generally only be + * accessed via get_main_ref_store(), as that will lazily initialize + * the ref object. + */ + struct ref_store *refs_private; /* * Contains path to often used file names. diff --git a/sha1-name.c b/sha1-name.c index 3aba62938f..c679e246cd 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1772,7 +1772,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo, cb.repo = repo; cb.list = &list; refs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb); - refs_head_ref(repo->refs, handle_one_ref, &cb); + refs_head_ref(get_main_ref_store(repo), handle_one_ref, &cb); commit_list_sort_by_date(&list); return get_oid_oneline(repo, name + 2, oid, list); }