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

From

Junio C Hamano <gitster@pobox.com>

Date

Sep 5, 2024, 21:20 UTC

Message-ID

<xmqqmsklztkd.fsf@gitster.g>

In-Reply-To

<xmqqr09xztqa.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> 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?

Perhaps like this?

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index f1bd31b2f7..27d181a612 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -328,7 +328,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
 	char *sparse_filename;
 	FILE *fp;
 	struct lock_file lk = LOCK_INIT;
-	int result;
+	int result = 0;
 
 	sparse_filename = get_sparse_checkout_filename();
 
@@ -336,19 +336,18 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		die(_("failed to create directory for sparse-checkout file"));
 
 	hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);
-	free(sparse_filename);
 
 	result = update_working_directory(pl);
 	if (result) {
 		rollback_lock_file(&lk);
 		clear_pattern_list(pl);
 		update_working_directory(NULL);
-		return result;
+		goto out;
 	}
 
 	fp = fdopen_lock_file(&lk, "w");
 	if (!fp)
-		die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
+		die_errno(_("unable to fdopen %s"), sparse_filename);
 
 	if (core_sparse_checkout_cone)
 		write_cone_to_file(fp, pl);
@@ -356,11 +355,13 @@ 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);
 
-	return 0;
+out:
+	free(sparse_filename);
+	return result;
 }
 
 enum sparse_checkout_mode {