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

[Junio C Hamano <gitster@xxxxxxxxx>]

Junio C Hamano <gitster@xxxxxxxxx> writes:

> Patrick Steinhardt <ps@xxxxxx> 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?

Of course not.  That would refer to a piece of memory that we
already free'ed in this function.

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 {