On Thu, Sep 05, 2024 at 02:20:18PM -0700, Junio C Hamano wrote:
> >> 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?
That works, though...
> 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);
Note the difference between "get_lock" and "get_locked" in these two.
The first will mention the tempfile name, and the second the destination
filename (and sparse_filename is the latter).
I don't know that it really matters much in practice, though. If
fdopen() fails it probably has nothing to do with the file itself, and
is either lack of memory or a programming bug (e.g., bogus descriptor).
I'll pick up this change, but I'll split the whole error-handling fix
into its own patch, since it's getting more complicated.
Will send v2 later tonight. Thanks, Patrick, for noticing the problem in
the first place.
-Peff
