threads / discuss / 58211

git write-tree segfault with core.untrackedCache true and nonexistent index

Subject: git write-tree segfault with core.untrackedCache true and nonexistent index

## tl;dr

6 messages between Jul 22, 2022 and Jul 22, 2022.

replies: 5people: 3as markdown or json

Joey Hess· Jul 22, 2022, 17:24 UTC · lore
	joey@darkstar:/tmp>git init emptyrepo
	Initialized empty Git repository in /tmp/emptyrepo/.git/
	joey@darkstar:/tmp>cd emptyrepo/
	joey@darkstar:/tmp/emptyrepo>git config core.untrackedCache true
	joey@darkstar:/tmp/emptyrepo>git write-tree
	Segmentation fault

I'm seeing this with git 2.37, 2.37.1, and HEAD. 2.36.1 does not have the problem.

Note that the index file does not exist prior to git write-tree in the above test case. When the index does exist, it doesn't segfault. Before, it would just generate the empty tree when the index did not exist.

Bisecting, e6a653554bb49c26d105f3b478cbdbb1c0648f65 is the first bad commit
commit e6a653554bb49c26d105f3b478cbdbb1c0648f65
Author: Tao Klerks <tao@klerks.biz>
Date:   Thu Mar 31 16:02:15 2022 +0000
    untracked-cache: support '--untracked-files=all' if configured
-- 
see shy jo
Martin Ågren· Jul 22, 2022, 19:25 UTC · re: Joey Hess · lore

Re: git write-tree segfault with core.untrackedCache true and nonexistent index

On Fri, 22 Jul 2022 at 20:20, Joey Hess <id@joeyh.name> wrote:
Show 8 quoted lines
>
>         joey@darkstar:/tmp>git init emptyrepo
>         Initialized empty Git repository in /tmp/emptyrepo/.git/
>         joey@darkstar:/tmp>cd emptyrepo/
>         joey@darkstar:/tmp/emptyrepo>git config core.untrackedCache true
>         joey@darkstar:/tmp/emptyrepo>git write-tree
>         Segmentation fault
>
[...]
Show 7 quoted lines
>
> Bisecting, e6a653554bb49c26d105f3b478cbdbb1c0648f65 is the first bad commit
> commit e6a653554bb49c26d105f3b478cbdbb1c0648f65
> Author: Tao Klerks <tao@klerks.biz>
> Date:   Thu Mar 31 16:02:15 2022 +0000
>
>     untracked-cache: support '--untracked-files=all' if configured
Thanks for a clear description, and for bisecting.

`repo` is NULL in `new_untracked_cache_flags()` and we're not prepared for that. The diff below fixes this in the sense that your reproducer stops failing, but I'm not sure it's the best approach.

I can't help but think that e6a653554b was just unlucky enough to dereference `istate->repo` and that the real issue is that we're missing

	if (!istate->repo)
		istate->repo = the_repository;

in some strategic place a fair bit earlier. It seems to me like the diff below is just papering over the real bug. It's not obvious to me where that check would want to go, though. Tao, do you have an idea?

Martin
--- a/dir.c
+++ b/dir.c
@@ -2752,6 +2752,9 @@ static unsigned new_untracked_cache_flags(struct index_state *istate)
 	struct repository *repo = istate->repo;
 	char *val;
 
+	if (!repo)
+		repo = the_repository;
+
 	/*
 	 * This logic is coordinated with the setting of these flags in
 	 * wt-status.c#wt_status_collect_untracked(), and the evaluation
-- 
2.37.1.455.g008518b4e5
Junio C Hamano· Jul 22, 2022, 19:41 UTC · re: Martin Ågren · lore

Re: git write-tree segfault with core.untrackedCache true and nonexistent index

Martin Ågren <martin.agren@gmail.com> writes:
Show 9 quoted lines
> I can't help but think that e6a653554b was just unlucky enough to
> dereference `istate->repo` and that the real issue is that we're missing
>
> 	if (!istate->repo)
> 		istate->repo = the_repository;
>
> in some strategic place a fair bit earlier. It seems to me like the diff
> below is just papering over the real bug. It's not obvious to me where
> that check would want to go, though. Tao, do you have an idea?
I am not Tao, but thanks for starting to analyze the real issue.

It seems that there are two public entry points to dir.c API that end up calling new_untracked_cache_flags().

One is read_directory(), which is the only caller of validate_untracked_cache() that calls new_untracked_cache_flags(). The callers of read_directory() are supposed to give istate, and it is quite unlikely they are throwing an istate with NULL in istate->repo, simply because read_directory() already makes abundant use of istate->repo.

The other one is add_untracked_cache().

Perhaps backtrace to see where the istate came from would quickly reveal where the real issue lies?

Thanks.
Martin Ågren· Jul 22, 2022, 21:22 UTC · re: Junio C Hamano · lore

[PATCH] read-cache: make `do_read_index()` always set up `istate->repo`

On Fri, 22 Jul 2022 at 21:41, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > I can't help but think that e6a653554b was just unlucky enough to
> > dereference `istate->repo` and that the real issue is that we're missing
> >
> >       if (!istate->repo)
> >               istate->repo = the_repository;
> >
> > in some strategic place a fair bit earlier. It seems to me like the diff
>
> Perhaps backtrace to see where the istate came from would quickly
> reveal where the real issue lies?

Here's an attempt at finding a proper spot for such a check. We end up with a small amount of duplicated code, but I think it should be ok, especially for a bugfix.

This is on top of tk/untracked-cache-with-uall and conflicts with 491df5f679 ("read-cache: set sparsity when index is new", 2022-05-10). The conflict could be resolved by taking my hunk before Victoria's [after which her helper function could be simplified accordingly] -- at least that passes all the tests.

I'm a bit out of my depth here. Hopefully one of the area experts can say more about this approach.

Martin
-- >8 --

If there is no index file, e.g., because the repository has just been created, we return zero early (unless `must_exist` makes us die instead.)

This early return means we do not set up `istate->repo`. With `core.untrackedCache=true`, the recent e6a653554b ("untracked-cache: support '--untracked-files=all' if configured", 2022-03-31) will eventually pass down `istate->repo` as a null pointer to `repo_config_get_string()`, causing a segmentation fault.

If we do hit this early return, set up `istate->repo` similar to when we actually read the index.

Reported-by: Joey Hess <id@joeyh.name>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 read-cache.c                      | 5 ++++-
 t/t7063-status-untracked-cache.sh | 6 ++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 3e0e7d4183..68ed65035b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2268,8 +2268,11 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	istate->timestamp.nsec = 0;
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
-		if (!must_exist && errno == ENOENT)
+		if (!must_exist && errno == ENOENT) {
+			if (!istate->repo)
+				istate->repo = the_repository;
 			return 0;
+		}
 		die_errno(_("%s: index file open failed"), path);
 	}
 
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 9936cc329e..7dc5631f3d 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -985,4 +985,10 @@ test_expect_success '"status" after file replacement should be clean with UC=fal
 	status_is_clean
 '
 
+test_expect_success 'empty repo (no index) and core.untrackedCache' '
+	git init emptyrepo &&
+	cd emptyrepo/ &&
+	git -c core.untrackedCache=true write-tree
+'
+
 test_done
-- 
2.37.1.455.g008518b4e5
Junio C Hamano· Jul 22, 2022, 21:46 UTC · re: Martin Ågren · lore

Re: [PATCH] read-cache: make `do_read_index()` always set up `istate->repo`

Martin Ågren <martin.agren@gmail.com> writes:
Show 34 quoted lines
> If there is no index file, e.g., because the repository has just been
> created, we return zero early (unless `must_exist` makes us die
> instead.)
>
> This early return means we do not set up `istate->repo`. With
> `core.untrackedCache=true`, the recent e6a653554b ("untracked-cache:
> support '--untracked-files=all' if configured", 2022-03-31) will
> eventually pass down `istate->repo` as a null pointer to
> `repo_config_get_string()`, causing a segmentation fault.
>
> If we do hit this early return, set up `istate->repo` similar to when we
> actually read the index.
>
> Reported-by: Joey Hess <id@joeyh.name>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  read-cache.c                      | 5 ++++-
>  t/t7063-status-untracked-cache.sh | 6 ++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 3e0e7d4183..68ed65035b 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2268,8 +2268,11 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>  	istate->timestamp.nsec = 0;
>  	fd = open(path, O_RDONLY);
>  	if (fd < 0) {
> -		if (!must_exist && errno == ENOENT)
> +		if (!must_exist && errno == ENOENT) {
> +			if (!istate->repo)
> +				istate->repo = the_repository;
>  			return 0;
> +		}
Makes sense.
Show 16 quoted lines
>  		die_errno(_("%s: index file open failed"), path);
>  	}
>  
> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
> index 9936cc329e..7dc5631f3d 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -985,4 +985,10 @@ test_expect_success '"status" after file replacement should be clean with UC=fal
>  	status_is_clean
>  '
>  
> +test_expect_success 'empty repo (no index) and core.untrackedCache' '
> +	git init emptyrepo &&
> +	cd emptyrepo/ &&
> +	git -c core.untrackedCache=true write-tree
> +'

I'll tweak this with "-C emptyrepo" so that future developers do not have to get bitten when they add more tests to this script.

THanks for a quick fix.  Very much appreciated.
>  test_done
Martin Ågren· Jul 22, 2022, 22:00 UTC · re: Junio C Hamano · lore

Re: [PATCH] read-cache: make `do_read_index()` always set up `istate->repo`

On Fri, 22 Jul 2022 at 23:46, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > +test_expect_success 'empty repo (no index) and core.untrackedCache' '
> > +     git init emptyrepo &&
> > +     cd emptyrepo/ &&
> > +     git -c core.untrackedCache=true write-tree
> > +'
>
> I'll tweak this with "-C emptyrepo" so that future developers do not
> have to get bitten when they add more tests to this script.

Yikes. I should have known better. Thanks for catching that. If a v2 is needed for any reason, I'll include this change.

Martin

← back to recent threads