new segfault in master (6a6c0f10a70a6eb1)

This test-tool submodule segfault seems new. Noticed it while checking dmesg for other things. There's also "name-rev HEAD~4000" (bottom), which is old, I think...

Core was generated by `$WT/t/helper/test-tool submodule-nested-repo-config submodule sub'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000559b12746174 in get_oid_with_context_1 (
    repo=repo@entry=0x7ffc5de3cf30, 
    name=name@entry=0x559b1280a882 ":.gitmodules", flags=flags@entry=0, 
    prefix=prefix@entry=0x0, oid=oid@entry=0x7ffc5de3ce80, 
    oc=oc@entry=0x7ffc5de3ce20) at sha1-name.c:1840
1840			if (!repo->index->cache)
(gdb) #0  0x0000559b12746174 in get_oid_with_context_1 (
    repo=repo@entry=0x7ffc5de3cf30, 
    name=name@entry=0x559b1280a882 ":.gitmodules", flags=flags@entry=0, 
    prefix=prefix@entry=0x0, oid=oid@entry=0x7ffc5de3ce80, 
    oc=oc@entry=0x7ffc5de3ce20) at sha1-name.c:1840
#1  0x0000559b12746dc3 in get_oid_with_context (oc=0x7ffc5de3ce20, 
    oid=0x7ffc5de3ce80, flags=0, str=str@entry=0x559b1280a882 ":.gitmodules", 
    repo=repo@entry=0x7ffc5de3cf30) at sha1-name.c:1946
#2  repo_get_oid (r=r@entry=0x7ffc5de3cf30, 
    name=name@entry=0x559b1280a882 ":.gitmodules", 
    oid=oid@entry=0x7ffc5de3ce80) at sha1-name.c:1595
#3  0x0000559b12753447 in config_from_gitmodules (
    fn=fn@entry=0x559b127534b0 <config_print_callback>, 
    repo=repo@entry=0x7ffc5de3cf30, data=0x559b145802c0)
    at submodule-config.c:633
#4  0x0000559b12754664 in print_config_from_gitmodules (
    repo=repo@entry=0x7ffc5de3cf30, key=<optimized out>)
    at submodule-config.c:742
#5  0x0000559b126dca4f in cmd__submodule_nested_repo_config (
    argc=<optimized out>, argv=0x7ffc5de3d290)
    at t/helper/test-submodule-nested-repo-config.c:27
#6  0x0000559b126d367f in cmd_main (argc=3, argv=0x7ffc5de3d290)
    at t/helper/test-tool.c:109
#7  0x0000559b126d337a in main (argc=4, argv=0x7ffc5de3d288)
    at common-main.c:50
(gdb) quit

Looks like a stack overflow:

Core was generated by `$WT/git name-rev HEAD~4000'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007f4df1896d6a in _int_malloc (
    av=av@entry=0x7f4df1bb7b00 <main_arena>, bytes=bytes@entry=33)
    at malloc.c:3444
(gdb) #0  0x00007f4df1896d6a in _int_malloc (
    av=av@entry=0x7f4df1bb7b00 <main_arena>, bytes=bytes@entry=33)
    at malloc.c:3444
#1  0x00007f4df1897b68 in malloc_check (sz=32, caller=<optimized out>)
    at hooks.c:295
#2  0x00005642d1240f51 in do_xmalloc (size=size@entry=32, 
    gentle=gentle@entry=0) at wrapper.c:60
#3  0x00005642d12410e7 in xmalloc (size=size@entry=32) at wrapper.c:87
#4  0x00005642d10c75b1 in name_rev (commit=0x5642d2357bb0, 
    tip_name=tip_name@entry=0x5642d245e7c0 "master", 
    taggerdate=taggerdate@entry=1000799900, generation=generation@entry=1044, 
    distance=distance@entry=1044, from_tag=from_tag@entry=0, deref=0)
    at builtin/name-rev.c:103
#5  0x00005642d10c74e3 in name_rev (commit=<optimized out>, 
    tip_name=tip_name@entry=0x5642d245e7c0 "master", 
    taggerdate=taggerdate@entry=1000799900, generation=generation@entry=1043, 
    distance=distance@entry=1043, from_tag=from_tag@entry=0, deref=0)
    at builtin/name-rev.c:138

<snip>...

#1047 0x00005642d10c74e3 in name_rev (commit=<optimized out>, 
    tip_name=tip_name@entry=0x5642d245e7c0 "master", 
    taggerdate=taggerdate@entry=1000799900, generation=generation@entry=1, 
    distance=distance@entry=1, from_tag=from_tag@entry=0, deref=0)
    at builtin/name-rev.c:138
#1048 0x00005642d10c74e3 in name_rev (commit=commit@entry=0x5642d22e6420, 
    tip_name=0x5642d245e7c0 "master", taggerdate=taggerdate@entry=1000799900, 
    generation=generation@entry=0, distance=distance@entry=0, 
    from_tag=from_tag@entry=0, deref=0) at builtin/name-rev.c:138
#1049 0x00005642d10c7889 in name_ref (path=<optimized out>, 
    oid=0x5642d2465bd8, flags=<optimized out>, cb_data=<optimized out>)
    at builtin/name-rev.c:276
#1050 0x00005642d11d5074 in do_for_each_repo_ref_iterator (
    r=0x5642d1546de0 <the_repo>, iter=0x5642d245da20, 
    fn=fn@entry=0x5642d11cab10 <do_for_each_ref_helper>, 
    cb_data=cb_data@entry=0x7ffc71fffa90) at refs/iterator.c:418
#1051 0x00005642d11cc7eb in do_for_each_ref (refs=<optimized out>, 
    prefix=prefix@entry=0x5642d12a0450 "", 
    fn=fn@entry=0x5642d10c75f0 <name_ref>, trim=trim@entry=0, 
    flags=flags@entry=0, cb_data=cb_data@entry=0x7ffc71fffb40) at refs.c:1496
#1052 0x00005642d11cd488 in refs_for_each_ref (cb_data=0x7ffc71fffb40, 
    fn=0x5642d10c75f0 <name_ref>, refs=<optimized out>) at refs.c:1502
#1053 for_each_ref (fn=fn@entry=0x5642d10c75f0 <name_ref>, 
    cb_data=cb_data@entry=0x7ffc71fffb40) at refs.c:1507
#1054 0x00005642d10c7ec5 in cmd_name_rev (argc=<optimized out>, 
    argv=0x7ffc71fffb40, prefix=<optimized out>) at builtin/name-rev.c:490
#1055 0x00005642d1070d68 in run_builtin (argv=<optimized out>, 
    argc=<optimized out>, p=<optimized out>) at git.c:444
#1056 handle_builtin (argc=2, argv=0x7ffc72000ac0) at git.c:675
#1057 0x00005642d1071d1e in run_argv (argv=0x7ffc72000840, 
    argcp=0x7ffc7200084c) at git.c:742
#1058 cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:876
#1059 0x00005642d107094a in main (argc=3, argv=0x7ffc72000ab8)
    at common-main.c:50
(gdb) quit

On Sat, May 11, 2019 at 08:57:11PM +0000, Eric Wong wrote:

Yeah, I hadn't seen it before. It's almost certainly the expect_failure added in 2b1257e463 (t/helper: add test-submodule-nested-repo-config, 2018-10-25), since otherwise we'd be complaining of a test failure.

I know we don't expect this to do the right thing yet, but it seems like there's still a bug, since the test seems to think we should output a nice message (and it's possible that the segfault can be triggered from non-test-tool code, too).

+cc the author.

Yeah, this one is old and expected. It's also in an expect_failure. The CI we run things through at GitHub complains if there are any segfaults, and I hacked around it with the patch below.

I sort of assumed nobody else cared, since they hadn't mentioned it. But we could do something similar. Though note in my version the default is "do not run the test", and we'd maybe want to flip it the other way (and also break up the setup step so that the succeeding test actually runs).

-- >8 --
Subject: [PATCH] t6120: mark a failing test with SEGFAULT_OK prereq

Upstream recently added a test of name-rev on a deep repository, which shows that its recursive algorithm can blow out the stack and segfault. We have several such tests already, but the twist here is that it's expect_failure. So we _know_ it's going to segfault and Git's test suite is OK with that, until the problem is fixed.

But our CI is not so forgiving. If it sees any segfault at all, it interrupts the test run and declares the whole thing a failure.

Let's just skip this test by adding a prerequisite that isn't filled. It's not telling us anything interesting. And if it ever gets fixed upstream, that will cause a conflict and we can start running it.

Note that we also have to skip the test after it, which relies on the state set up by the first one. This isn't a big deal, as it's not testing code that we're likely to change ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t6120-describe.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Sat, May 11, 2019 at 06:31:20PM -0400, Jeff King wrote:

Actually, the plot thickens. That test _used to_ correctly expect failure (well, sort of -- it greps for the string with %s, which is wrong!). But then more recently in d9b8b8f896 (submodule-config.c: use repo_get_oid for reading .gitmodules, 2019-04-16), it started actually doing the lookup in the correct repo. And that started the segfault, because nobody has actually loaded the index for the submodule.

I don't think this can be triggered outside of test-tool. There are four ways to get to config_from_gitmodules():

  - via repo_read_gitmodules(), which explicitly loads the index

  - via print_config_from_gitmodules(). This is called from
    submodule--helper, but only with the_repository as the argument (and
    I _think_ that the_repository->index is never NULL, because we point
    it at the_index).

  - via fetch_config_from_gitmodules(), which always passes
    the_repository

  - via update_clone_config_from_gitmodules(), likewise

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).

I'm out of time for now, but I'll look into cleaning this up and writing a real commit message later.

---

On Sun, May 12, 2019 at 6:02 AM Jeff King <peff@peff.net> wrote:

I did miss this test. Yeah your fix makes sense.

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

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(-)

Jeff King <peff@peff.net> wrote:

Awesome, looks obviously correct and can confirm it fixes the new segfault :>

Now I'm kinda wondering if some static checker coulda/shoulda spotted that, sooner.

On Tue, May 14, 2019 at 8:54 PM Jeff King <peff@peff.net> wrote:

We could even drop the "if" and call repo_read_index() unconditionally. If the index is already read, it will be no-op (forcing a reread has always been discard_index(); read_index();)

Thanks for catching this by the way. I'll need to go through all the_index conversion to see if I left similar traps like this.

On Wed, May 15, 2019 at 08:24:34AM +0700, Duy Nguyen wrote:

I think you missed my bit after the "---":

  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. :)

Yeah. I could not find any others, but it would not hurt to have a second set of eyes.

Also from my earlier message, if you missed it:

  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.

-Peff

Jeff King <peff@peff.net> writes:

That is an approach to make it harder to make mistakes by accepting possibly a small wasted resource; but at that point, I think calling repo_read_index() unconditionally from here and similar places would be a simpler fix in the same spirit.

On Wed, May 15, 2019 at 12:16 PM Junio C Hamano <gitster@pobox.com> wrote:

For repo_read_index() case, maybe. But we have a lot of "r(epo)->index->something". All or most of these references traditionally are the_index.something, which is always safe to dereference. Submodule repos with the optionally NULL repo->index break this assumption.

So either we audit all the code and make sure "repo->index" cannot be NULL because repo_read_index() has been called before (and may have to repeat auditing), or we just stick to the old assumption and make sure repo->index is not NULL from the beginning. This makes me think the small extra resource is worth it. Much less time will be spent on similar issues now and in the future.

PS. Sorry Jeff for the noise. I should have waited until I come home and can read your mail more carefully.

Duy Nguyen <pclouds@gmail.com> writes:

Ah, good point.  Thanks for a dose of sanity.

There are two ways a 'struct repository' could be initialized before using: via initialize_the_repository() and repo_init().

The first way always initializes 'index' field because that's how it is before the introduction of 'struct repository'. Back then 'the_index' is always available (even if not loaded). The second way however leaves 'index' NULL and relies on repo_read_index() to allocate it on demand.

The problem with the second way is that, the majority of our code base was written with 'the_index' (i.e. the first way) in mind, where dereferencing 'the_index' (or the 'index' field now) is always safe.

The second way breaks this assumption. The 'index' field can be NULL until loading from disk, which could lead to segfaults like 581d2fd9f2 (get_oid: handle NULL repo->index, 2019-05-14).

We have two options to handle this: either we audit the entire code base, adding 'is index NULL' when needed, or we make sure 'index' is never NULL to begin with.

This patch goes with the second option, making sure that 'index' is always allocated after initialization. It's less effort than the first one, and also safer because you could still miss things during the code audit. The extra allocation cost is not a real concern.

The 'index' field is still freed and reset to NULL in repo_clear(). But after that call, a lot more is missing in 'repo' and it can never be used again without going through reinitialization phase. So it should be fine.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 repository.c | 3 ++-
 repository.h | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

On Sun, May 19, 2019 at 09:56:36AM +0700, Nguyễn Thái Ngọc Duy wrote:

I think this direction makes sense.

The patch looks good, though I wonder if we could simplify even further by just embedding an index into the repository object. The purpose of having it as a pointer, I think, is so that the_repository can point to the_index. But we could possibly hide the latter behind some macro trickery like:

  #define the_index (the_repository->index)

I spent a few minutes on a proof of concept patch, but it gets a bit hairy:

  1. There are some circular dependencies in the header files. We'd need
     repository.h to depend on cache.h to get the definition of
     index_state, but the latter includes repository.h. We'd need to
     break the index bits out of cache.h into index.h, which in turn
     requires breaking out some other parts. I did a sloppy job of it in
     the patch below.

  2. There are hundreds of spots that need to swap out "repo->index" for
     "&repo->index". In the patch below I just did enough to compile
     archive-zip.o, to illustrate. :)

So it's definitely non-trivial to go that way. I'm not sure if it's worth the effort to switch at this point, but even if it is, your patch seems like a good thing to do in the meantime.

Either way, I think we could probably revert the non-test portion of my 581d2fd9f2 (get_oid: handle NULL repo->index, 2019-05-14) after this.

-Peff

---

On Mon, May 20, 2019 at 8:17 PM Jeff King <peff@peff.net> wrote:

You are more thorough than me. I saw #2 first and immediately backed off (partly for a selfish reason: I have plenty of the_repo conversion patches in queue and anything touching "repo" may delay those patches even more).

There's also #3 but this one is minor. So far 'struct repo' is more of a glue of things. Embedding index_state in it while leaving object_store, ref_store... pointers feels inconsistent and a bit weird. It's not a strong reason for making index_state a pointer too, but if we have to deal with pointers anyway...

Yeah. I'm thinking of doing that after, scanning for similar lines too. But it looks like it's the only one. Will fix in v2.

On Tue, May 21, 2019 at 05:34:02PM +0700, Duy Nguyen wrote:

Yeah, that's true, it would be disruptive.

And yeah, I agree it would nice for it to all be consistent. Let's leave it at your patch for now, and we can think about refactoring this later.

-Peff

Duy Nguyen <pclouds@gmail.com> writes:

Yeah, the fact that the_reopsitory->index is not an embedded instance has bothered me from the very beginning, and I am happy to see others share the same feeling ;-)

Thanks.