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

From: Jeff King <peff@peff.net>
Date: Sep 6, 2024, 01:19 UTC
Message-ID: <20240906011935.GA2616101@coredump.intra.peff.net>
In-Reply-To: <xmqqmsklztkd.fsf@gitster.g>

On Thu, Sep 05, 2024 at 02:20:18PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

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

Show 13 quoted lines

>  	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