[PATCH v3] object-name: avoid use-after-free in get_oid_with_context_1()

[Shlok Kulshreshtha <diy2903@gmail.com>]

When a ":<path>" argument names a relative path, resolve_relative_path()
returns a newly allocated string and "cp" is pointed at it:

	new_path = resolve_relative_path(repo, cp);
	if (!new_path) {
		namelen = namelen - (cp - name);
	} else {
		cp = new_path;
		namelen = strlen(cp);
	}

From there on "cp" and "new_path" name the same allocation. Later the
memory location that "new_path" points to is freed.

	free(new_path);
	if (reject_tree_in_index(repo, only_to_die, ce, stage, prefix, cp))

But here the reject_tree_in_index() passes "cp" to
diagnose_invalid_index_path(), which calls strlen() on it, looks it up
in the index, and formats it into its messages, allocating as it goes.
All of this reads memory that has already been freed.

Collapse the two exits into one to ensure a single free() that happens
after the last use.

Three things have to coincide to reach this:

1. The path has to be relative, or nothing is allocated and "cp"
still points into the argument.

2. The entry found has to be a sparse
directory, which needs a sparse index.

3. The argument has to get past the check in die_verify_filename() that
skips a leading ':' followed by a non-alphanumeric, so ":0:./dir/"
arrives here where ":./dir/" does not.

Add a test to t1092 that covers the combination. It fails under
SANITIZE=address without the change to object-name.c.

This was reported in [1], and the shape used here was suggested in
review [2], but that series was not rerolled and the fix never landed.

[1] https://lore.kernel.org/git/cf6bcdb43e5b4abab464c30a914d64dc8e7a9925.1655336146.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/xmqqy1xxw7rc.fsf@gitster.g/

Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Original-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Helped-by: Junio C Hamano <gitster@pobox.com>
Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Shlok Kulshreshtha <diy2903@gmail.com>
---
Changes since v2: apply Patrick's suggestion to assign
reject_tree_in_index()'s return value directly, since it only
returns 0 or -1, which is exactly what this function needs to return
too. No change to the test. This reduces redundancy.

The three conditions make this awkward to reach by hand, so here is the
recipe:

	git init sparse && cd sparse &&
	mkdir folder1 folder2 &&
	echo a >folder1/a && echo b >folder2/b &&
	git add -A && git commit -m init &&
	git sparse-checkout init --cone --sparse-index &&
	git sparse-checkout set folder1 &&
	git show :0:./folder2/

Without the change below, no sanitizer is needed to see it. On 2.52.0
the buffer has already been reused by the time the message is formatted,
so the path printed is whatever now sits in that memory, and it differs
from run to run:

	fatal: path '' does not exist (neither on disk nor in the index)
	fatal: path 'M-6?:xM-@M-:M-L??X' does not exist (neither on disk nor in the index)
	fatal: path '?M-*JM-^M->M-YM-tn?H' does not exist (neither on disk nor in the index)

Still without the change, built with SANITIZE=address, the same command
reports

	ERROR: AddressSanitizer: heap-use-after-free
	READ of size 3 at 0x607000002a20
	    #1 diagnose_invalid_index_path object-name.c:1653
	    #2 get_oid_with_context_1      object-name.c:1807
	    #3 maybe_die_on_misspelt_object_name
	    #4 die_verify_filename         setup.c:216
	    #6 setup_revisions             revision.c:3103
	    #8 cmd_show                    log.c:694
	freed by thread T0 here:
	    #1 get_oid_with_context_1      object-name.c:1806
	previously allocated by thread T0 here:
	    #5 prefix_path                 setup.c:149
	    #6 get_oid_with_context_1      object-name.c:1784

With the change, the message reads "folder2/" every time and the
sanitizer stays quiet.

"git diff" and "git rev-parse" reach it the same way, and so does "../"
from a subdirectory.

Re-verified against the current tree: the t1092 test still fails
without the object-name.c change and passes with it, under
SANITIZE=address.
 object-name.c                            | 14 ++++++++------
 t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/object-name.c b/object-name.c
index 83efba0ba6..026ff8c6dd 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1803,13 +1803,15 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
 			    memcmp(ce->name, cp, namelen))
 				break;
 			if (ce_stage(ce) == stage) {
+				int ret = reject_tree_in_index(repo, only_to_die, ce,
+							       stage, prefix, cp);
+
+				if (!ret) {
+					oidcpy(oid, &ce->oid);
+					oc->mode = ce->ce_mode;
+				}
 				free(new_path);
-				if (reject_tree_in_index(repo, only_to_die, ce,
-							 stage, prefix, cp))
-					return -1;
-				oidcpy(oid, &ce->oid);
-				oc->mode = ce->ce_mode;
-				return 0;
+				return ret;
 			}
 			pos++;
 		}
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 446c1776cb..05b54062b3 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1405,6 +1405,17 @@ do
 	"
 done
 
+test_expect_success 'relative path to a sparse directory' '
+	init_repos &&
+
+	# A ":<stage>:<path>" argument whose path is relative is resolved
+	# into a heap-allocated buffer, and a sparse directory found at that
+	# path is reported through it.  Cover that combination, so that the
+	# reporting does not read the buffer after it has been released.
+	test_sparse_match test_must_fail git show :0:./folder1/ &&
+	test_sparse_match test_must_fail git rev-parse :0:./folder1/
+'
+
 test_expect_success 'submodule handling' '
 	init_repos &&
 
-- 
2.52.0