git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: Eric Wong <e@80x24.org>, Antonio Ospite <ao2@ao2.it>,
	Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org
Subject: [PATCH] get_oid: handle NULL repo->index
Date: Tue, 14 May 2019 09:54:55 -0400	[thread overview]
Message-ID: <20190514135455.GA17927@sigill.intra.peff.net> (raw)
In-Reply-To: <20190511230204.GA18474@sigill.intra.peff.net>

On Sat, May 11, 2019 at 07:02:05PM -0400, Jeff King wrote:

> But regardless, I think it makes sense to load the index on demand when
> we need it here, which makes Antonio's original test pass (like the
> patch below).
> 
> The segfault ultimately comes from repo_get_oid(); we feed it
> ":.gitmodules" and it blindly looks at repo->index. It's probably worth
> it being a bit more defensive and just returning "no such entry" if
> there's no index to look at (it could also load on demand, I guess, but
> it seems like too low a level to be making that kind of decision).

This turned out to be even simpler than the patch I posted earlier.
After polishing up my submodule-config fix, I looked at teaching
get_oid() to behave differently. And it turns out that it already tries
to load the index on demand! There's just a silly mistake in the code to
check whether the index is already initialized.

Once we fix that, we don't need to handle this specifically in the
submodule code. So here's a simpler, revised patch:

-- >8 --
Subject: [PATCH] get_oid: handle NULL repo->index

When get_oid() and its helpers see an index name like ":.gitmodules",
they try to load the index on demand, like:

  if (repo->index->cache)
	repo_read_index(repo);

However, that misses the case when "repo->index" itself is NULL; we'll
segfault in the conditional.

This never happens with the_repository; there we always point its index
field to &the_index. But a submodule repository may have a NULL index
field until somebody calls repo_read_index().

This bug is triggered by t7411, but it was hard to notice because it's
in an expect_failure block. That test was added by 2b1257e463 (t/helper:
add test-submodule-nested-repo-config, 2018-10-25). Back then we had no
easy way to access the .gitmodules blob of a submodule repo, so we
expected (and got) an error message to that effect. Later, d9b8b8f896
(submodule-config.c: use repo_get_oid for reading .gitmodules,
2019-04-16) started looking in the correct repo, which is when we
started triggering the segfault.

With this fix, the test starts passing (once we clean it up as its
comment instructs).

Note that as far as I know, this bug could not be triggered outside of
the test suite. It requires resolving an index name in a submodule, and
all of the code paths (aside from test-tool) which do that either load
the index themselves, or always pass the_repository.

Ultimately it comes from 3a7a698e93 (sha1-name.c: remove implicit
dependency on the_index, 2019-01-12), which replaced a check of
"the_index.cache" with "repo->index->cache". So even if there is another
way to trigger it, it wouldn't affect any versions before then.

Signed-off-by: Jeff King <peff@peff.net>
---
Arguably this code should just unconditionally call repo_read_index(),
which should be a noop if the index is already loaded. But I wanted to
do the minimal fix here, without getting into any subtle differences
between what checking index->cache versus index->initialized might mean.
Anybody who wants to dig into that is welcome to make a patch on top. :)

I also wondered if we should simply allocate an empty index whenever we
have a non-toplevel "struct repository", which might be less surprising
to other callers. I don't have a strong opinion either way. I did grep
around for other callers which might have similar problems, but couldn't
find any.

 sha1-name.c                 | 2 +-
 t/t7411-submodule-config.sh | 8 ++------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/sha1-name.c b/sha1-name.c
index 775a73d8ad..455e9fb1ea 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1837,7 +1837,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
 		if (flags & GET_OID_RECORD_PATH)
 			oc->path = xstrdup(cp);
 
-		if (!repo->index->cache)
+		if (!repo->index || !repo->index->cache)
 			repo_read_index(repo);
 		pos = index_name_pos(repo->index, cp, namelen);
 		if (pos < 0)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index fcc0fb82d8..ad28e93880 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -243,18 +243,14 @@ test_expect_success 'reading nested submodules config' '
 	)
 '
 
-# When this test eventually passes, before turning it into
-# test_expect_success, remember to replace the test_i18ngrep below with
-# a "test_must_be_empty warning" to be sure that the warning is actually
-# removed from the code.
-test_expect_failure 'reading nested submodules config when .gitmodules is not in the working tree' '
+test_expect_success 'reading nested submodules config when .gitmodules is not in the working tree' '
 	test_when_finished "git -C super/submodule checkout .gitmodules" &&
 	(cd super &&
 		echo "./nested_submodule" >expect &&
 		rm submodule/.gitmodules &&
 		test-tool submodule-nested-repo-config \
 			submodule submodule.nested_submodule.url >actual 2>warning &&
-		test_i18ngrep "nested submodules without %s in the working tree are not supported yet" warning &&
+		test_must_be_empty warning &&
 		test_cmp expect actual
 	)
 '
-- 
2.21.0.1388.g2b1efd806f


  parent reply	other threads:[~2019-05-14 13:54 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-11 20:57 new segfault in master (6a6c0f10a70a6eb1) Eric Wong
2019-05-11 22:31 ` Jeff King
2019-05-11 23:02   ` Jeff King
2019-05-12  4:26     ` Duy Nguyen
2019-05-14 13:54     ` Jeff King [this message]
2019-05-14 23:38       ` [PATCH] get_oid: handle NULL repo->index Eric Wong
2019-05-15  1:24       ` Duy Nguyen
2019-05-15  1:46         ` Jeff King
2019-05-15  5:16           ` Junio C Hamano
2019-05-15  9:29             ` Duy Nguyen
2019-05-16  1:43               ` Junio C Hamano
2019-05-19  2:56                 ` [PATCH] repository.c: always allocate 'index' at repo init time Nguyễn Thái Ngọc Duy
2019-05-20 13:17                   ` Jeff King
2019-05-21 10:34                     ` Duy Nguyen
2019-05-21 20:58                       ` Jeff King
2019-05-28 16:07                       ` Junio C Hamano

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190514135455.GA17927@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=ao2@ao2.it \
    --cc=e@80x24.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --subject='Re: [PATCH] get_oid: handle NULL repo->index' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).