From: René Scharfe Date: Sat, 08 Aug 2026 16:23:33 GMT Subject: Re: [PATCH] object-name: avoid use-after-free in get_oid_with_context_1() Message-ID: <506880fc-5557-46a1-b26d-63349781849b@web.de> In-Reply-To: <20260807195941.99473-1-diy2903@gmail.com> On 8/7/26 9:59 PM, Shlok Kulshreshtha wrote: > When a ":" 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/ Oh, from 2022, good find. > Reported-by: Johannes Schindelin Original-patch-by even, no? > Helped-by: Junio C Hamano > Signed-off-by: Shlok Kulshreshtha > --- > 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 "::" 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é