On Thu, Sep 05, 2024 at 04:27:49AM -0400, Jeff King wrote:
> 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.
> @@ -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
