[PATCH] fixup! propagate errno from failing read

From: Han-Wen Nienhuys <redacted>

This fixes a crash triggered by the BUG() statement.

This can occur with symlinked .git/refs. To check availability,
refs_verify_refname_available will run refs_read_raw_ref() on each prefix,
leading to a read() from .git/refs (which is a directory).

When handling the symlink case, it is probably more robust to re-issue the
lstat() as a normal stat(), in which case, we would fall back to the directory
case.

For now, propagating errno from strbuf_read() is simpler and avoids the crash.

Signed-off-by: Han-Wen Nienhuys <redacted>
---
    fixup! propagate errno from failing read
    
    This fixes a crash triggered by the BUG() statement.
    
    This can occur with symlinked .git/refs. To check availability,
    refs_verify_refname_available will run refs_read_raw_ref() on each
    prefix, leading to a read() from .git/refs (which is a directory).
    
    When handling the symlink case, it is probably more robust to re-issue
    the lstat() as a normal stat(), in which case, we would fall back to the
    directory case.
    
    For now, propagating errno from strbuf_read() is simpler and avoids the
    crash.
    
    Signed-off-by: Han-Wen Nienhuys hanwen@google.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1068%2Fhanwen%2Ffiles-fixup-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1068/hanwen/files-fixup-v1
Pull-Request: https://github.com/git/git/pull/1068

 refs/files-backend.c |  1 +
 t/t3200-branch.sh    | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0d96eeba61b..f546cc3cc3d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -454,6 +454,7 @@ stat_ref:
 	}
 	strbuf_reset(&sb_contents);
 	if (strbuf_read(&sb_contents, fd, 256) < 0) {
+		myerr = errno;
 		close(fd);
 		goto out;
 	}

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cc4b10236e2..dd17718160a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -109,6 +109,20 @@ test_expect_success 'git branch -m n/n n should work' '
 	git reflog exists refs/heads/n
 '
 
+test_expect_success 'git branch -m with symlinked .git/refs' '
+	git init subdir &&
+	test_when_finished "rm -rf subdir" &&
+	(cd subdir &&
+		for d in refs objects packed-refs ; do
+		rm -rf .git/$d &&
+		ln -s ../../.git/$d .git/$d ; done ) &&
+	git --git-dir subdir/.git/ branch rename-src &&
+	expect=$(git rev-parse rename-src) &&
+	git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
+	test $(git rev-parse rename-dest) = "$expect" &&
+	git branch -D rename-dest
+'
+
 # The topmost entry in reflog for branch bbb is about branch creation.
 # Hence, we compare bbb@{1} (instead of bbb@{0}) with aaa@{0}.
 

base-commit: f000ecbed922c727382651490e75014f003c89ca
-- 
gitgitgadget

Reviewed-by: Jonathan Tan <redacted>

Thanks - a straightforward fixup. (I don't think we need the errno from
close() in this case.)

Hi,

Jonathan Tan wrote:

Han-Wen Nienhuys wrote:

quoted

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0d96eeba61b..f546cc3cc3d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -454,6 +454,7 @@ stat_ref:
 	}
 	strbuf_reset(&sb_contents);
 	if (strbuf_read(&sb_contents, fd, 256) < 0) {
+		myerr = errno;
 		close(fd);
 		goto out;
 	}

Reviewed-by: Jonathan Tan <redacted>

Thanks - a straightforward fixup. (I don't think we need the errno from
close() in this case.)

This looks good as far as it goes, but how do we know this has covered
all the code paths?  Let's see.

The only nonobvious paths are

 stat_ref:
	/*
	 * We might have to loop back here to avoid a race
	 * condition: first we lstat() the file, then we try
	 * to read it as a link or as a file.  But if somebody
	 * changes the type of the file (file <-> directory
	 * <-> symlink) between the lstat() and reading, then
	 * we don't want to report that as an error but rather
	 * try again starting with the lstat().
	 *
	 * We'll keep a count of the retries, though, just to avoid
	 * any confusing situation sending us into an infinite loop.
	 */

	if (remaining_retries-- <= 0)
		goto out;

and

	ret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);

 out:
	if (ret && !myerr)
		BUG("returning non-zero %d, should have set myerr!", ret);

For the 'stat_ref' case, we have to check that whenever we 'goto
stat_ref', we set myerr moments before.  Fortunately, that is the
case.

For the 'fall through into out' case, we have the check the
parse_loose_ref_contents always sets *failure_errno on error.  That is
also the case.

So this indeed covers all our cases, and the BUG now correctly
reflects an invariant we can count on.  Thanks for the fix, and thanks
for looking it over.

Sincerely,
Jonathan

On Tue, Aug 17, 2021 at 12:31:29PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

This test presumably needs the SYMLINKS prerequisite. I noticed that the
Windows CI for "next" is now failing.

-Peff

d1931bcf0d (refs: make errno output explicit for refs_resolve_ref_unsafe,
2021-07-20) add a test for a crash when refs is a symlink, but it fails
on windows.

add the missing SYMLINKS dependency and while at it, refactor it slightly
to comply better with the CodingGuidelines.

Reported-by: Jeff King <redacted>
Signed-off-by: Carlo Marcelo Arenas Belón <redacted>
---
 t/t3200-branch.sh | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd17718160..6d8700664e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -109,17 +109,22 @@ test_expect_success 'git branch -m n/n n should work' '
 	git reflog exists refs/heads/n
 '
 
-test_expect_success 'git branch -m with symlinked .git/refs' '
+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
 	git init subdir &&
 	test_when_finished "rm -rf subdir" &&
-	(cd subdir &&
-		for d in refs objects packed-refs ; do
-		rm -rf .git/$d &&
-		ln -s ../../.git/$d .git/$d ; done ) &&
+	(
+		cd subdir &&
+		for d in refs objects packed-refs
+		do
+			rm -rf .git/$d &&
+			ln -s ../../.git/$d .git/$d
+		done
+	) &&
 	git --git-dir subdir/.git/ branch rename-src &&
-	expect=$(git rev-parse rename-src) &&
+	git rev-parse rename-src >expect &&
 	git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
-	test $(git rev-parse rename-dest) = "$expect" &&
+	git rev-parse rename-dest >actual &&
+	test_cmp expect actual &&
 	git branch -D rename-dest
 '
 

base-commit: d1931bcf0d5ef75cdaf836347f4aefce902a6a38
-- 
2.33.0.476.gf000ecbed9

On Thu, Aug 19, 2021 at 1:02 AM Carlo Marcelo Arenas Belón
[off-list ref] wrote:

Ideally, the above line should be:

    ln -s ../../.git/$d .git/$d || exit 1

to catch and signal a failure within the for-loop body (which happens
to be in a subshell; if not in a subshell, you'd use `|| return 1`).

+               done
+       ) &&
        git --git-dir subdir/.git/ branch rename-src &&
-       expect=$(git rev-parse rename-src) &&
+       git rev-parse rename-src >expect &&
        git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
-       test $(git rev-parse rename-dest) = "$expect" &&
+       git rev-parse rename-dest >actual &&
+       test_cmp expect actual &&
        git branch -D rename-dest
 '

d1931bcf0d (refs: make errno output explicit for refs_resolve_ref_unsafe,
2021-07-20) add a test for a crash when refs is a symlink, but it fails
on windows.

add the missing SYMLINKS dependency and while at it, refactor it slightly
to comply better with the CodingGuidelines.

Reported-by: Jeff King <redacted>
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Carlo Marcelo Arenas Belón <redacted>
---
v2:
* update subject for clarity; might be worth squashing instead into 3d63ce75e
  (refs: explicitly return failure_errno from parse_loose_ref_contents,
  2021-07-20)
* change from --git-dir to -C for clarity
* add reporting for errors to the for loop as suggested by Eric

 t/t3200-branch.sh | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd17718160..eacb6bcd35 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -109,17 +109,22 @@ test_expect_success 'git branch -m n/n n should work' '
 	git reflog exists refs/heads/n
 '
 
-test_expect_success 'git branch -m with symlinked .git/refs' '
+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
 	git init subdir &&
 	test_when_finished "rm -rf subdir" &&
-	(cd subdir &&
-		for d in refs objects packed-refs ; do
-		rm -rf .git/$d &&
-		ln -s ../../.git/$d .git/$d ; done ) &&
-	git --git-dir subdir/.git/ branch rename-src &&
-	expect=$(git rev-parse rename-src) &&
-	git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
-	test $(git rev-parse rename-dest) = "$expect" &&
+	(
+		cd subdir &&
+		for d in refs objects packed-refs
+		do
+			rm -rf .git/$d &&
+			ln -s ../../.git/$d .git/$d || exit 1
+		done
+	) &&
+	git -C subdir branch rename-src &&
+	git rev-parse rename-src >expect &&
+	git -C subdir branch -m rename-src rename-dest &&
+	git rev-parse rename-dest >actual &&
+	test_cmp expect actual &&
 	git branch -D rename-dest
 '
 

base-commit: d1931bcf0d5ef75cdaf836347f4aefce902a6a38
-- 
2.33.0.476.gf000ecbed9