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

On 8/7/26 9:59 PM, Shlok Kulshreshtha wrote:
> 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/[email protected]/

Oh, from 2022, good find.

> Reported-by: Johannes Schindelin <[email protected]>

Original-patch-by even, no?

> Helped-by: Junio C Hamano <[email protected]>
> Signed-off-by: Shlok Kulshreshtha <[email protected]>
> ---
> 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.
> 
> The new t1092 test fails without the object-name.c hunk and passes with
> it, under SANITIZE=address.
>  object-name.c                            | 15 +++++++++------
>  t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++++
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/object-name.c b/object-name.c
> index 83efba0ba6..bffe795830 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -1803,13 +1803,16 @@ 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 = -1;
> +
> +				if (!reject_tree_in_index(repo, only_to_die, ce,
> +							  stage, prefix, cp)) {
> +					oidcpy(oid, &ce->oid);
> +					oc->mode = ce->ce_mode;
> +					ret = 0;
> +				}
>  				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;

OK

>  			}
>  			pos++;
>  		}
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 4140c4d8ef..e88946c254 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1357,6 +1357,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 &&
>  

Good idea to add a test.

René