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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 5, 2024, 10:54 UTC
Message-ID: <ZtmN6sZCvDy1BGfn@pks.im>
In-Reply-To: <20240905082749.GA88220@coredump.intra.peff.net>

On Thu, Sep 05, 2024 at 04:27:49AM -0400, Jeff King wrote:

Show 14 quoted lines

> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 2604ab04df..f1bd31b2f7 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -336,8 +335,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
>  	if (safe_create_leading_directories(sparse_filename))
>  		die(_("failed to create directory for sparse-checkout file"));
>  
> -	fd = hold_lock_file_for_update(&lk, sparse_filename,
> -				      LOCK_DIE_ON_ERROR);
> +	hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);
>  	free(sparse_filename);
>  
>  	result = update_working_directory(pl);

Okay, we die on an error so there is no need to check `fd`, either. Makes sense.

Show 20 quoted lines

> @@ -348,15 +346,17 @@ static int write_patterns_and_update(struct pattern_list *pl)
>  		return result;
>  	}
>  
> -	fp = xfdopen(fd, "w");
> +	fp = fdopen_lock_file(&lk, "w");
> +	if (!fp)
> +		die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
>  
>  	if (core_sparse_checkout_cone)
>  		write_cone_to_file(fp, pl);
>  	else
>  		write_patterns_to_file(fp, pl);
>  
> -	fflush(fp);
> -	commit_lock_file(&lk);
> +	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.

Other than that this patch looks sensible. Also kind of curious that we never checked that `commit_lock_file()` was successful, so we wouldn't even notice when renaming the file into place failed.

Patrick