patchsparse-checkout: use fdopen_lock_file() instead of xfdopen()

When updating sparse patterns, we open a lock_file to write out the new data. The lock_file struct holds the file descriptor, but we call fdopen() to get a stdio handle to do the actual write.

After we finish writing, we fflush() so that all of the data is on disk, and then call commit_lock_file() which closes the descriptor. But we never fclose() the stdio handle, leaking it.

The obvious solution seems like it would be to just call fclose(). But when? If we do it before commit_lock_file(), then the lock_file code is left thinking it owns the now-closed file descriptor, and will do an extra close() on the descriptor. But if we do it before, we have the opposite problem: the lock_file code will close the descriptor, and fclose() will do the extra close().

We can handle this correctly by using fdopen_lock_file(). That leaves ownership of the stdio handle with the lock_file, which knows not to double-close it.

We do have to adjust the code a bit:

  - we have to handle errors ourselves; we can just die(), since that's
    what xfdopen() would have done (and we can even provide a more
    specific error message).

  - we no longer need to call fflush(); committing the lock-file
    auto-closes it, which will now do the flush for us. As a bonus, this
    will actually check that the flush was successful before renaming
    the file into place. Let's likewise report when committing the lock
    fails (rather than quietly returning success from the command).

  - we can get rid of the local "fd" variable, since we never look at it
    ourselves now

Signed-off-by: Jeff King <peff@peff.net>
---
Curiously, this is not reported as a leak by LSan, and it is included in
the "reachable" set by valgrind. I imagine this is because libc has to
maintain a list of open writable handles, since fflush(NULL) is supposed
to flush all of them. I peeked at other fdopen(..., "w") calls to see if
there were other similar spots, but didn't notice any.

I found this because I was building git on an Android system, and they have an "fdsan" that complains when the underlying descriptor of a handle is closed. It flagged some other spots, too, but the rest that I looked at involved the tempfile atexit() handler closing the descriptors manually (we don't care about flushing there, as our goal is to just prevent the "can't delete an open file" problem on Windows).

 builtin/sparse-checkout.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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

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

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

Patrick Steinhardt <ps@pks.im> writes:

Hmph.  Would this be sufficient as a band-aid, then?

 builtin/sparse-checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Junio C Hamano <gitster@pobox.com> writes:

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

Perhaps like this?

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

That works, though...

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

Jeff King <peff@peff.net> writes:

I did consider ... to write %s.%s", sparse_filename, LOCK_SUFFIX) but then thought that the final name is what is more relevant to the end user. Yes, while correcting unrelated error, I shouldn't have tried to improve unrelated end-user experience ;-).

Yeah, thanks, both.

Jeff King <peff@peff.net> writes:

OK.  The neessary change is surprisingly small.

Sounds like fun.

Will queue.  Thanks.

Here's a re-roll that fixes the use-after-free of the lock filename that Patrick noticed. I pulled the error-checking fix into its own patch (patch 2 here), and did Junio's suggested "goto out" as preparation in patch 1. Patch 3 is the leak fix.

Range diff is below, but it's much harder to read than just looking at the updated patch 3.

Thanks both for review on round 1.

  [1/3]: sparse-checkout: consolidate cleanup when writing patterns
  [2/3]: sparse-checkout: check commit_lock_file when writing patterns
  [3/3]: sparse-checkout: use fdopen_lock_file() instead of xfdopen()

 builtin/sparse-checkout.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

-:  ---------- > 1:  03a80d3748 sparse-checkout: consolidate cleanup when writing patterns
-:  ---------- > 2:  20628aa350 sparse-checkout: check commit_lock_file when writing patterns
1:  b4700ba1a9 ! 3:  fe41f9f02b sparse-checkout: use fdopen_lock_file() instead of xfdopen()
    @@ Commit message
           - we no longer need to call fflush(); committing the lock-file
             auto-closes it, which will now do the flush for us. As a bonus, this
             will actually check that the flush was successful before renaming
    -        the file into place. Let's likewise report when committing the lock
    -        fails (rather than quietly returning success from the command).
    +        the file into place.
     
           - we can get rid of the local "fd" variable, since we never look at it
             ourselves now
    @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_l
     -	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);
    + 	if (result) {
     @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_list *pl)
    - 		return result;
    + 		goto out;
      	}
      
     -	fp = xfdopen(fd, "w");
    @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_l
      		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);
    + 	if (commit_lock_file(&lk))
    + 		die_errno(_("unable to write %s"), sparse_filename);
      

In write_patterns_and_update(), we always need to free the pattern list before exiting the function. Rather than handling it manually when we return early, we can jump to an "out" label where cleanup happens. This let us drop one line, but also establishes a pattern we can use for other cleanup.

Signed-off-by: Jeff King <peff@peff.net>
---
This would also be helpful in the long run if all of the other die()
calls ended up as error returns, but I guess this is in builtin/, so it
may not be that likely a candidate for libification anyway.

 builtin/sparse-checkout.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

When writing a new "sparse-checkout" file, we do the usual strategy of writing to a lockfile and committing it into place. But we don't check the outcome of commit_lock_file(). Failing there would prevent us from writing a bogus file (good), but we would ignore the error and return a successful exit code (bad).

Fix this by calling die(). Note that we need to keep the sparse_filename variable valid for longer, since the filename stored in the lock_file struct will be dropped when we run commit_lock_file().

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/sparse-checkout.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

When updating sparse patterns, we open a lock_file to write out the new data. The lock_file struct holds the file descriptor, but we call fdopen() to get a stdio handle to do the actual write.

After we finish writing, we fflush() so that all of the data is on disk, and then call commit_lock_file() which closes the descriptor. But we never fclose() the stdio handle, leaking it.

The obvious solution seems like it would be to just call fclose(). But when? If we do it before commit_lock_file(), then the lock_file code is left thinking it owns the now-closed file descriptor, and will do an extra close() on the descriptor. But if we do it before, we have the opposite problem: the lock_file code will close the descriptor, and fclose() will do the extra close().

We can handle this correctly by using fdopen_lock_file(). That leaves ownership of the stdio handle with the lock_file, which knows not to double-close it.

We do have to adjust the code a bit:

  - we have to handle errors ourselves; we can just die(), since that's
    what xfdopen() would have done (and we can even provide a more
    specific error message).

  - we no longer need to call fflush(); committing the lock-file
    auto-closes it, which will now do the flush for us. As a bonus, this
    will actually check that the flush was successful before renaming
    the file into place.

  - we can get rid of the local "fd" variable, since we never look at it
    ourselves now

Signed-off-by: Jeff King <peff@peff.net>
---
I left this error message using get_lock_file_path(&lk), which is still
valid, and is arguably correct than sparse_filename.

 builtin/sparse-checkout.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

On Thu, Sep 05, 2024 at 11:45:57PM -0400, Jeff King wrote:

Thanks for going the extra steps! This version looks good to me.

Patrick