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

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

[Joey Hess]

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

	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Martin Ågren]

On Fri, 22 Jul 2022 at 20:20, Joey Hess <id@joeyh.name> wrote:
>
>         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
>
[...]
>
> 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

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

[Junio C Hamano]

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

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

[Martin Ågren]

On Fri, 22 Jul 2022 at 21:41, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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

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

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

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

>  		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

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

[Martin Ågren]

On Fri, 22 Jul 2022 at 23:46, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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