[PATCH] read_gitfile_gently(): return non-repo path on error

[PATCH] read_gitfile_gently(): return non-repo path on error

[Jeff King]

This patch fixes a potential segfault when resolving a .git file that
points to an invalid path. The bug was introduced by 1dd27bfbfd (setup:
improve error diagnosis for invalid .git files, 2026-03-04).

In setup_git_directory_gently() we call read_gitfile_gently(), which may
return a numeric code to us on error. If die_on_error is set, we then
feed that code to read_gitfile_error_die(), which also wants the path to
the gitfile and, in the case of ERROR_NOT_A_REPO, the non-repo directory
that the gitfile pointed to.

But we don't have that pointed-to directory available, so we just pass
NULL. That ends up calling die("not a git repository: %s", NULL). This
may crash, though on many systems (like glibc) it will just print
"(null)". So even if we don't crash, we're generating nonsense output.

The problem comes from 1dd27bfbfd. Before that, when die_on_error was
set we'd pass NULL to read_gitfile_gently()'s return_error_code
parameter, which means it would call read_gitfile_error_die() itself.
And it _does_ have that pointed-to directory as a string, and correctly
passes it.  But since 1dd27bfbfd, we always get the numeric error code
back from read_gitfile_gently(), and then decide whether to call
read_gitfile_error_die() in the caller. And since we don't have the
"dir" parameter, we just pass NULL.

Unfortunately the fix is not a simple matter of passing the string to
the right function. We have to get it out of read_gitfile_gently() in
the first place, which means we have to return it as another
out-parameter. And because it involves allocating memory, we can't just
do so unconditionally; callers need to be ready to free it after
handling the error.

I've tried to make the minimally-invasive fix here:

  1. We only copy the string when we hit READ_GITFILE_ERR_NOT_A_REPO,
     so other error codes don't have to worry about freeing it.

  2. We'll turn read_gitfile_gently() into a wrapper which passes NULL
     by default, leaving other callers unaffected.

The result is kind of gross. There's an extra layer of macro
indirection, and the validity of the string is subtly tied to the
NOT_A_REPO error. A cleaner solution might be an error struct that
couples the code and the output string together, along with a function
to free the error struct. But then all callers would have to be modified
to call the free function. Alternatively, we could perhaps put a
large-ish fixed-size buffer in the struct, though that means potential
truncation and a larger stack footprint in each caller (even when they
don't have see an error).

So I've left that as possible work for the future, or maybe never. Some
of this gross-ness was already there. For example, the only other caller
of read_gitfile_error_die() is in submodule.c, and it also passes NULL
for the "dir" parameter. But it does so only when the code is not
NOT_A_REPO! So it is depending on the same subtle connection to avoid
triggering the bug.

There's an existing test in t0002 which triggers this case, but we
didn't notice the problem because it checks only that we said "not a
repository", and not the full string. So if we print "(null)" it is
happy. It will probably crash on some non-glibc platforms, but nobody
seems to have reported it yet (the breakage is recent-ish as of v2.54).
I'm also somewhat surprised that building with ASan/UBSan doesn't catch
this, but it doesn't seem to (and I found an open issue with somebody
asking for it to be implemented in the sanitizers).

We can beef up the test by checking for the full string, which does
demonstrate the bug.

Signed-off-by: Jeff King <peff@peff.net>
---
Two other points of interest.

One, I'm not sure how useful printing the pointed-to directory is. We
_could_ just say:

  fatal: gitfile does not point to a valid repository: /path/to/.git

which is enough for somebody to investigate themselves. That would
certainly make the patch smaller.

And two, I ran into this running doc-diff:

  $ ./doc-diff HEAD^ HEAD
  fatal: not a git repository: (null)

The correct output (which this patch produces) is:

  fatal: not a git repository: /home/peff/compile/git/.git/worktrees/worktree3

And indeed, that path is missing. But why? I feel like I've run into
this same problem occasionally over the last year or so, but never
before. Did we get more aggressive about removing worktrees at some
point? I haven't been able to reproduce whatever is killing off the
worktree directory, and by the time I see the error it is long gone.

Anyway, that's not strictly related to this bug, but just how I
happened across it.

 setup.c            | 19 +++++++++++++++----
 setup.h            |  3 ++-
 t/t0002-gitfile.sh |  2 +-
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/setup.c b/setup.c
index 075bf89fa9..2df6fbf595 100644
--- a/setup.c
+++ b/setup.c
@@ -955,8 +955,14 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
  * will be set to an error code and NULL will be returned. If
  * return_error_code is NULL the function will die instead (for most
  * cases).
+ *
+ * If the code is READ_GITFILE_ERR_NOT_A_REPO and return_error_dir is
+ * non-NULL, the directory to which the gitfile points will be returned
+ * there. The caller is responsible for freeing the resulting string.
  */
-const char *read_gitfile_gently(const char *path, int *return_error_code)
+const char *read_gitfile_gently_with_error_dir(const char *path,
+					       int *return_error_code,
+					       char **return_error_dir)
 {
 	const int max_file_size = 1 << 20;  /* 1MB */
 	int error_code = 0;
@@ -1021,6 +1027,8 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	}
 	if (!is_git_directory(dir)) {
 		error_code = READ_GITFILE_ERR_NOT_A_REPO;
+		if (return_error_dir)
+			*return_error_dir = xstrdup(dir);
 		goto cleanup_return;
 	}
 
@@ -1613,11 +1621,12 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		int offset = dir->len, error_code = 0;
 		char *gitdir_path = NULL;
 		char *gitfile = NULL;
+		char *error_dst = NULL;
 
 		if (offset > min_offset)
 			strbuf_addch(dir, '/');
 		strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
-		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
+		gitdirenv = read_gitfile_gently_with_error_dir(dir->buf, &error_code, &error_dst);
 		if (!gitdirenv) {
 			switch (error_code) {
 			case READ_GITFILE_ERR_MISSING:
@@ -1641,9 +1650,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 					return GIT_DIR_INVALID_GITFILE;
 			default:
 				if (die_on_error)
-					read_gitfile_error_die(error_code, dir->buf, NULL);
-				else
+					read_gitfile_error_die(error_code, dir->buf, error_dst);
+				else {
+					free(error_dst);
 					return GIT_DIR_INVALID_GITFILE;
+				}
 			}
 		} else {
 			gitfile = xstrdup(dir->buf);
diff --git a/setup.h b/setup.h
index 7878c9d267..65f55d5268 100644
--- a/setup.h
+++ b/setup.h
@@ -39,7 +39,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_MISSING 9
 #define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
-const char *read_gitfile_gently(const char *path, int *return_error_code);
+const char *read_gitfile_gently_with_error_dir(const char *path, int *return_error_code, char **return_error_dir);
+#define read_gitfile_gently(path, err) read_gitfile_gently_with_error_dir((path), (err), NULL)
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
 const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
 #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index dfbcdddbcc..6967e12b9f 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -27,7 +27,7 @@ test_expect_success 'bad setup: invalid .git file format' '
 test_expect_success 'bad setup: invalid .git file path' '
 	echo "gitdir: $REAL.not" >.git &&
 	test_must_fail git rev-parse 2>.err &&
-	test_grep "not a git repository" .err
+	test_grep "not a git repository: $REAL.not" .err
 '
 
 test_expect_success 'final setup + check rev-parse --git-dir' '
-- 
2.54.0.682.g2f9b59d445

Re: [PATCH] read_gitfile_gently(): return non-repo path on error

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I've tried to make the minimally-invasive fix here:
>
>   1. We only copy the string when we hit READ_GITFILE_ERR_NOT_A_REPO,
>      so other error codes don't have to worry about freeing it.
>
>   2. We'll turn read_gitfile_gently() into a wrapper which passes NULL
>      by default, leaving other callers unaffected.

Nice, probably.  I do not know what to feel about the first point,
though, as it burdens those who add new callers in the future more.

> The result is kind of gross. There's an extra layer of macro
> indirection, and the validity of the string is subtly tied to the
> NOT_A_REPO error. A cleaner solution might be an error struct that
> couples the code and the output string together, along with a function
> to free the error struct. But then all callers would have to be modified
> to call the free function. Alternatively, we could perhaps put a
> large-ish fixed-size buffer in the struct, though that means potential
> truncation and a larger stack footprint in each caller (even when they
> don't have see an error).

None of thoese are particularly appetizing ;-).

> So I've left that as possible work for the future, or maybe never. Some
> of this gross-ness was already there. For example, the only other caller
> of read_gitfile_error_die() is in submodule.c, and it also passes NULL
> for the "dir" parameter. But it does so only when the code is not
> NOT_A_REPO! So it is depending on the same subtle connection to avoid
> triggering the bug.

Yup.  I can agree with this.

> ---
> Two other points of interest.
>
> One, I'm not sure how useful printing the pointed-to directory is. We
> _could_ just say:
>
>   fatal: gitfile does not point to a valid repository: /path/to/.git
>
> which is enough for somebody to investigate themselves. That would
> certainly make the patch smaller.

Thanks.  While reading the main explanation, it was the first thing
that came to me.

The implementation and the test are as expected in patches from you
and matches the intent explained in the log message exactly.

Thanks, will queue.

Re: [PATCH] read_gitfile_gently(): return non-repo path on error

[Jeff King]

On Tue, Jun 02, 2026 at 04:42:15PM +0900, Junio C Hamano wrote:

> > One, I'm not sure how useful printing the pointed-to directory is. We
> > _could_ just say:
> >
> >   fatal: gitfile does not point to a valid repository: /path/to/.git
> >
> > which is enough for somebody to investigate themselves. That would
> > certainly make the patch smaller.
> 
> Thanks.  While reading the main explanation, it was the first thing
> that came to me.

Here's what that looks like, for reference. It is nice and simple, if we
think the change in error message is acceptable. I hate to change
user-facing error messages because of internal code details, but I
really do wonder if the existing message is the most useful thing to
print in the first place.

diff --git a/setup.c b/setup.c
index 075bf89fa9..ed86671d84 100644
--- a/setup.c
+++ b/setup.c
@@ -920,7 +920,7 @@ int verify_repository_format(const struct repository_format *format,
 	return 0;
 }
 
-void read_gitfile_error_die(int error_code, const char *path, const char *dir)
+void read_gitfile_error_die(int error_code, const char *path)
 {
 	switch (error_code) {
 	case READ_GITFILE_ERR_NOT_A_FILE:
@@ -940,7 +940,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
 	case READ_GITFILE_ERR_NO_PATH:
 		die(_("no path in gitfile: %s"), path);
 	case READ_GITFILE_ERR_NOT_A_REPO:
-		die(_("not a git repository: %s"), dir);
+		die(_("gitfile does not point to a valid repository: %s"),
+		    path);
 	default:
 		BUG("unknown error code");
 	}
@@ -1031,7 +1032,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	if (return_error_code)
 		*return_error_code = error_code;
 	else if (error_code)
-		read_gitfile_error_die(error_code, path, dir);
+		read_gitfile_error_die(error_code, path);
 
 	free(buf);
 	return error_code ? NULL : path;
@@ -1641,7 +1642,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 					return GIT_DIR_INVALID_GITFILE;
 			default:
 				if (die_on_error)
-					read_gitfile_error_die(error_code, dir->buf, NULL);
+					read_gitfile_error_die(error_code, dir->buf);
 				else
 					return GIT_DIR_INVALID_GITFILE;
 			}
diff --git a/setup.h b/setup.h
index 7878c9d267..436aaa22c1 100644
--- a/setup.h
+++ b/setup.h
@@ -38,7 +38,7 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_TOO_LARGE 8
 #define READ_GITFILE_ERR_MISSING 9
 #define READ_GITFILE_ERR_IS_A_DIR 10
-void read_gitfile_error_die(int error_code, const char *path, const char *dir);
+void read_gitfile_error_die(int error_code, const char *path);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
 const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
diff --git a/submodule.c b/submodule.c
index a939ff5072..c36732ca0b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2578,7 +2578,7 @@ void absorb_git_dir_into_superproject(const char *path,
 
 		if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
 			/* We don't know what broke here. */
-			read_gitfile_error_die(err_code, path, NULL);
+			read_gitfile_error_die(err_code, path);
 
 		/*
 		* Maybe populated, but no git directory was found?
diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index dfbcdddbcc..6356e9ec72 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -27,7 +27,7 @@ test_expect_success 'bad setup: invalid .git file format' '
 test_expect_success 'bad setup: invalid .git file path' '
 	echo "gitdir: $REAL.not" >.git &&
 	test_must_fail git rev-parse 2>.err &&
-	test_grep "not a git repository" .err
+	test_grep "gitfile does not point to a valid repository" .err
 '
 
 test_expect_success 'final setup + check rev-parse --git-dir' '

Re: [PATCH] read_gitfile_gently(): return non-repo path on error

[Patrick Steinhardt]

On Tue, Jun 02, 2026 at 02:11:59AM -0400, Jeff King wrote:
[snip]
> Two other points of interest.
> 
> One, I'm not sure how useful printing the pointed-to directory is. We
> _could_ just say:
> 
>   fatal: gitfile does not point to a valid repository: /path/to/.git
> 
> which is enough for somebody to investigate themselves. That would
> certainly make the patch smaller.

I have to agree that the patch is somewhat gross, and I myself don't
really see much of an issue to move to an error message like the above
if it ends up simplifying the logic.

> And two, I ran into this running doc-diff:
> 
>   $ ./doc-diff HEAD^ HEAD
>   fatal: not a git repository: (null)
> 
> The correct output (which this patch produces) is:
> 
>   fatal: not a git repository: /home/peff/compile/git/.git/worktrees/worktree3
> 
> And indeed, that path is missing. But why? I feel like I've run into
> this same problem occasionally over the last year or so, but never
> before. Did we get more aggressive about removing worktrees at some
> point? I haven't been able to reproduce whatever is killing off the
> worktree directory, and by the time I see the error it is long gone.

Both git-gc(1) and git-maintenance(1) prune orphaned worktrees that are
older than three months by default, which can be configured via
"gc.worktreePruneExpire". That logic has changed in 4dda60c9df (Merge
branch 'ps/maintenance-missing-tasks', 2025-05-15), which would kind of
match your timeline.

But rereading that patch series I cannot really see how it could result
in more aggressive pruning of worktrees. We used `git worktree prune
--expire <expiry>` before that series, and we still use that logic now.

Hum.

> diff --git a/setup.c b/setup.c
> index 075bf89fa9..2df6fbf595 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1641,9 +1650,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  					return GIT_DIR_INVALID_GITFILE;
>  			default:
>  				if (die_on_error)
> -					read_gitfile_error_die(error_code, dir->buf, NULL);
> -				else
> +					read_gitfile_error_die(error_code, dir->buf, error_dst);
> +				else {
> +					free(error_dst);
>  					return GIT_DIR_INVALID_GITFILE;
> +				}

The `if` branch should also gain some curly braces here.

Patrick