Null pointer dereference in git-submodule

Null pointer dereference in git-submodule

[Jeremy Feusi]

[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]

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

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} "$@"

cheers
Jeremy

[-- Attachment #2: min-man.tar.gz --]
[-- Type: application/gzip, Size: 42292 bytes --]

Re: Null pointer dereference in git-submodule

[René Scharfe]

Am 24.03.2018 um 18:42 schrieb Jeremy Feusi:
> 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
> 
> 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 <jeremy@feusi.co>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/submodule--helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..0f74e81005 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -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);
 
-- 
2.16.3

Re: Null pointer dereference in git-submodule

[Jeremy Feusi]

Reply-To: 

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

cheers
Jeremy

In-Reply-To: <a7ad9dbf-1b0f-efc6-3a17-51cf25381ce5@web.de>

On Sat, Mar 24, 2018 at 09:42:57PM +0100, René Scharfe wrote:
> Am 24.03.2018 um 18:42 schrieb Jeremy Feusi:
> > 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
> > 
> > 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 <jeremy@feusi.co>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/submodule--helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ee020d4749..0f74e81005 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -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);
>  
> -- 
> 2.16.3

Re: Null pointer dereference in git-submodule

[René Scharfe]

Am 25.03.2018 um 11:50 schrieb Jeremy Feusi:
> 
> 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 get_submodule_ref_store()

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 <jeremy@feusi.co>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Test missing..

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..ae3014ac5a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -654,9 +654,13 @@ 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) {
+			print_status(flags, '-', path, ce_oid, displaypath);
+			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

Re: Null pointer dereference in git-submodule

[Stefan Beller]

On Sun, Mar 25, 2018 at 3:58 AM René Scharfe <l.s.r@web.de> wrote:

> Am 25.03.2018 um 11:50 schrieb Jeremy Feusi:
> >
> > 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?

At first I thought we could still retrieve the ref store via a lookup of
path -> name in .gitmodules and then navigate to
.git/modules<name>/ (as seen from the superproject)
and load the ref store. But loading the refstore is a mere detail.

"not initialized" is technically correct, the existing git directory
inside the superproject doesn't matter.


> -- >8 --
> 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

> 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!

> Reported-by: Jeremy Feusi <jeremy@feusi.co>

Please also sign off instead of just claiming to report it.
(The sign off has legal implications, see
https://developercertificate.org/ or our copy in
Documentation/SubmittingPatches)

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Test missing..

Which would be added in t/t7400-submodule-basic.sh

Thanks for coming up with a sensible patch!
Stefan


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

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ee020d4749..ae3014ac5a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -654,9 +654,13 @@ 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) {
> +                       print_status(flags, '-', path, ce_oid,
displaypath);
> +                       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

Re: Null pointer dereference in git-submodule

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> 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
>
>> 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!

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

Thanks.

Re: Null pointer dereference in git-submodule

[Stefan Beller]

On Wed, Mar 28, 2018 at 9:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> 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
>>
>>> 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!
>
> 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

Rereading the discussion, I overlooked the author of the second patch
to be Rene (for some reason I thought someone else would have
written that patch. Sorry about that!)

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

As Rene likes. I can reroll that patch with a test, too.
I'd pick it up after rerolling the series from yesterday
(moving nested submodules).

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

Thanks,
Stefan