Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

From

Junio C Hamano <gitster@pobox.com>

Date

Sep 5, 2024, 21:16 UTC

Message-ID

<xmqqr09xztqa.fsf@gitster.g>

In-Reply-To

<ZtmN6sZCvDy1BGfn@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

>> +	if (commit_lock_file(&lk))
>> +		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
>>  
>>  	clear_pattern_list(pl);
>
> I think the error handling is broken. `commit_lock_file()` calls
> `rename_tempfile()`, which deletes the temporary file even in the error
> case. The consequence is that `lk->tempfile` will be set to the `NULL`
> pointer. When we call `get_locked_file_path()` we then dereference it
> unconditionally and would thus segfault.

Hmph.  Would this be sufficient as a band-aid, then?

 builtin/sparse-checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/builtin/sparse-checkout.c w/builtin/sparse-checkout.c
index f1bd31b2f7..60363fd056 100644
--- c/builtin/sparse-checkout.c
+++ w/builtin/sparse-checkout.c
@@ -356,7 +356,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		write_patterns_to_file(fp, pl);
 
 	if (commit_lock_file(&lk))
-		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
+		die_errno(_("unable to write %s"), sparse_filename);
 
 	clear_pattern_list(pl);