From: Jeff King Date: Tue, 2 Jun 2026 06:11:59 +0000 (-0400) Subject: read_gitfile_gently(): return non-repo path on error X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=6d110c4afa2874c39d71a770d48dda104ffd9b60;p=thirdparty%2Fgit.git read_gitfile_gently(): return non-repo path on error 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 Signed-off-by: Junio C Hamano --- diff --git a/setup.c b/setup.c index 7ec4427368..4fb401650a 100644 --- a/setup.c +++ b/setup.c @@ -952,8 +952,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; @@ -1018,6 +1024,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; } @@ -1605,11 +1613,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: @@ -1633,9 +1642,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 80bc6e5f07..0c87830e62 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' '