Null pointer dereference in git-submodule

Am 24.03.2018 um 18:42 schrieb Jeremy Feusi:

Quoted 32 lines

Hi,
While bootstrapping a gnu repository I noticed that git segfaulted when called as "git submodule status". After compiling git with address sanitizer and minimizing the directory I finally narrowed it down to the files which I have attached as a tar archive. Here is a detailed backtrace:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==63400==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000c27a93 bp 0x7ffdcb4eec10 sp 0x7ffdcb4eeb80 T0) ==63400==The signal is caused by a READ memory access.
==63400==Hint: address points to the zero page.

     #0 0xc27a92 in refs_read_raw_ref /home/jfe/git/refs.c:1451:20
     #1 0xc174a6 in refs_resolve_ref_unsafe /home/jfe/git/refs.c:1493:7
     #2 0xc1826a in refs_read_ref_full /home/jfe/git/refs.c:224:6
     #3 0xc26d53 in refs_head_ref /home/jfe/git/refs.c:1314:7
     #4 0x8071e6 in status_submodule /home/jfe/git/builtin/submodule--helper.c:658:7
     #5 0x806a89 in status_submodule_cb /home/jfe/git/builtin/submodule--helper.c:699:2
     #6 0x80523e in for_each_listed_submodule /home/jfe/git/builtin/submodule--helper.c:438:3
     #7 0x7f7e9a in module_status /home/jfe/git/builtin/submodule--helper.c:732:2
     #8 0x7efd69 in cmd_submodule__helper /home/jfe/git/builtin/submodule--helper.c:1859:11
     #9 0x51e024 in run_builtin /home/jfe/git/git.c:346:11
     #10 0x5192c2 in handle_builtin /home/jfe/git/git.c:554:8
     #11 0x51d0f0 in run_argv /home/jfe/git/git.c:606:4
     #12 0x518600 in cmd_main /home/jfe/git/git.c:683:19
     #13 0x8501d6 in main /home/jfe/git/common-main.c:43:9
     #14 0x7f49fdaf2f29 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20f29)
     #15 0x41f4b9 in _start (/home/jfe/git/inst/libexec/git-core/git+0x41f4b9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/jfe/git/refs.c:1451:20 in refs_read_raw_ref ==63400==ABORTING

Quoted 9 lines

As mentioned above, this bug is triggered by issuing the command "git submodule status" while in the attached directory.

This bug was confirmed on Debian with version 2.16.1 and 2.17.0.rc1.35.g90bbd502d as well as on Arch Linux with version 2.16.2 where further output is given by git:

/usr/lib/git-core/git-submodule: line 979: 8119 Segmentation fault (core dumped) git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} "$@"

You may have minimized too much. With the patch below I get:

fatal: no ref store in submodule 'gnulib'

I guess you'll get a different one in your original repo.

The patch seems like a good idea in any case, though.

-- >8 --
Subject: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

refs_head_ref() requires a valid ref_store pointer to be given as its first argument. get_submodule_ref_store() can return NULL. Exit and report the failure to find a ref store in that case instead of segfaulting.

Reported-by Jeremy Feusi <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
 builtin/submodule--helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

builtin/submodule--helper.c +4 -2

index ee020d4749..0f74e81005 100644
@@ -654,9 +654,11 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 			     displaypath);
 	} else if (!(flags & OPT_CACHED)) {
 		struct object_id oid;
+		struct ref_store *refs = get_submodule_ref_store(path);
 
-		if (refs_head_ref(get_submodule_ref_store(path),
-				  handle_submodule_head_ref, &oid))
+		if (!refs)
+			die(_("no ref store in submodule '%s'"), path);
+		if (refs_head_ref(refs, handle_submodule_head_ref, &oid))
 			die(_("could not resolve HEAD ref inside the "
 			      "submodule '%s'"), path);
 

Signature · 1 line

2.16.3

Hmm... That's weird. I can reproduce it on 3 independant systems with versions 2.16.2 up, although it does not work with version 2.11.0. Anyway, I figured out how to reproduce this bug. It is caused when a submodule is added and then the directory it resides in is moved or deleted without commiting. For example:

git init
git submodule add https://github.com/git/git git
mv git git.BAK
git submodule status #this command segfaults

Am 25.03.2018 um 11:50 schrieb Jeremy Feusi:

Quoted 11 lines

Hmm... That's weird. I can reproduce it on 3 independant systems with versions 2.16.2 up, although it does not work with version 2.11.0. Anyway, I figured out how to reproduce this bug. It is caused when a submodule is added and then the directory it resides in is moved or deleted without commiting. For example:

git init
git submodule add https://github.com/git/git git
mv git git.BAK
git submodule status #this command segfaults

With the patch I sent in my first reply the last command reports:

fatal: no ref store in submodule 'git'

That may not be the most helpful message -- not just the ref store is missing, the whole submodule is gone!

Come to think about it, this removal may be intended. How about showing the submodule as not being initialized at that point?

-- >8 --
Subject: [PATCH v2] submodule: check for NULL return of

If we can't find a ref store for a submodule then assume it the latter is not initialized (or was removed). Print a status line accordingly instead of causing a segmentation fault by passing NULL as the first parameter of refs_head_ref().

Reported-by Jeremy Feusi <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
Test missing..

builtin/submodule--helper.c +3 -1

+                       goto cleanup;
+               }
+               if (refs_head_ref(refs, handle_submodule_head_ref, &oid))
                         die(_("could not resolve HEAD ref inside the "
                               "submodule '%s'"), path);
 
--

2.17.0.rc1.38.g7c51fd80b8

Quoted 1 line

Subject: [PATCH v2] submodule: check for NULL return of

get_submodule_ref_store()

Maybe more imperative, telling what we actually want to achieve instead of what we do?

submodule: report deleted submodules as not initialized

Quoted 4 lines

If we can't find a ref store for a submodule then assume it the latter is not initialized (or was removed). Print a status line accordingly instead of causing a segmentation fault by passing NULL as the first parameter of refs_head_ref().

Thanks for the message here. Looks good!
...
Which would be added in t/t7400-submodule-basic.sh

Thanks for coming up with a sensible patch!

Quoted 17 lines · Stefan Beller

Quoted 1 line

Subject: [PATCH v2] submodule: check for NULL return of

get_submodule_ref_store()

Maybe more imperative, telling what we actually want to achieve instead of what we do?

submodule: report deleted submodules as not initialized

Quoted 4 lines

If we can't find a ref store for a submodule then assume it the latter is not initialized (or was removed). Print a status line accordingly instead of causing a segmentation fault by passing NULL as the first parameter of refs_head_ref().

Thanks for the message here. Looks good!
...
Which would be added in t/t7400-submodule-basic.sh

Quoted 1 line

Thanks for coming up with a sensible patch!

I take the above to mean that you as a contributor active in this area like the general idea in the patch but not volunteering to take this topic over

and instead trust René to tie the loose ends with a reroll, taking hints from your suggestions?

I just wanted to make sure that we won't be confused whose turn it is next (e.g. me waiting for update to t7400 from you or René doing the same).

t/t7400-submodule-basic.sh +2 -0

+       )
+'

t/t7400-submodule-basic.sh test_expect_success 'moving the superproject does not break submodules' ' · line 829

Quoted 11 lines · Stefan Beller

t/t7400-submodule-basic.sh +9 -0

@@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not break submodules' '
+test_expect_success 'moving the submodule does not break the superproject' '
+       (
+               cd addtest2 &&
+
+               mv repo repo.bak &&
+               git submodule status >actual &&
+               grep -e "^-" -e repo actual &&
+
+               mv repo.bak repo

Should this "move back" be encapsulated in a test_when_finished?

Quoted 11 lines · Eric Sunshine

t/t7400-submodule-basic.sh test_expect_success 'moving the superproject does not break submodules' ' · line 832

Quoted 9 lines

t/t7400-submodule-basic.sh +9 -0

+test_expect_success 'moving the submodule does not break the superproject' '
+       (
+               cd addtest2 &&
+
+               mv repo repo.bak &&
+               git submodule status >actual &&
+               grep -e "^-" -e repo actual &&
+
+               mv repo.bak repo

Should this "move back" be encapsulated in a test_when_finished?

I thought about that, but decided against it for some reason as I was debating where to put the test_when_finished. I mostly saw those at the very beginning of a test and wondered if it can be called from within a subshell. (I'd not want to put it at the beginning but rather adjacent to the move.)

Or, even simpler:

    mv addtest2/repo addtest2/repo.bak &&
    test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
    git -C addtest2 submodule status >actual &&
    grep -e "^-" -e repo actual

Quoted 13 lines · Eric Sunshine

Quoted 11 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_expect_success 'moving the submodule does not break the superproject' '
+       (
+               cd addtest2 &&
+
+               mv repo repo.bak &&
+               git submodule status >actual &&
+               grep -e "^-" -e repo actual &&
+
+               mv repo.bak repo

Should this "move back" be encapsulated in a test_when_finished?

Quoted 4 lines

I thought about that, but decided against it for some reason as I was debating where to put the test_when_finished. I mostly saw those at the very beginning of a test and wondered if it can be called from within a subshell. (I'd not want to put it at the beginning but rather adjacent to the move.)

It looks like test_when_finished() shouldn't be used in a subshell. However, wouldn't the following be reasonable?

     mv addtest2/repo addtest2/repo.bak &&
     test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
     (
         cd addtest2 &&
         git submodule status >actual &&
         grep -e "^-" -e repo actual
     )

Or, even simpler:

     mv addtest2/repo addtest2/repo.bak &&
     test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
     git -C addtest2 submodule status >actual &&
     grep -e "^-" -e repo actual

This looks nicer here in the script, but doesn't test exactly what users type most of the time, I suppose.

So how about this?

+ sed -e "s/^ \([^ ]* repo\) .*/-\1/" <actual >expect &&

Quoted 4 lines

This looks nicer here in the script, but doesn't test exactly what users type most of the time, I suppose.

So how about this?

Looks good to me, though I had a nagging feeling at first that the regex could be made more concise.
Why do we need the optional "[^ ]" inside \1 ?

Quoted 1 line

+ sed -e "s/^ \([^ ]* repo\) .*/-\1/" <actual >expect &&

Quoted 4 lines

This looks nicer here in the script, but doesn't test exactly what users type most of the time, I suppose.

So how about this?

Looks good to me, though I had a nagging feeling at first that the regex could be made more concise.
Why do we need the optional "[^ ]" inside \1 ?

Quoted 1 line

+ sed -e "s/^ \([^ ]* repo\) .*/-\1/" <actual >expect &&