git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 3/3] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

From
Jeff King <peff@peff.net>
Date
Sep 6, 2024, 03:48 UTC
Message-ID
<20240906034841.GC4168362@coredump.intra.peff.net>
In-Reply-To
<20240906034557.GA3693911@coredump.intra.peff.net>

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(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index b5e220cc44..5ccf696862 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -327,7 +327,6 @@ static int write_patterns_and_update(struct pattern_list *pl)
 {
 	char *sparse_filename;
 	FILE *fp;
-	int fd;
 	struct lock_file lk = LOCK_INIT;
 	int result;
 
@@ -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);
 
 	result = update_working_directory(pl);
 	if (result) {
@@ -346,14 +344,15 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		goto out;
 	}
 
-	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);
 	if (commit_lock_file(&lk))
 		die_errno(_("unable to write %s"), sparse_filename);
 
-- 
2.46.0.802.g13da1a47c4
Previous: Jeff KingNext: Patrick Steinhardt
Message 11 of 12 in “sparse-checkout: use fdopen_lock_file() instead of xfdopen()”
  1. sparse-checkout: use fdopen_lock_file() instead of xfdopen()Jeff King, Sep 5, 2024
  2. Patrick SteinhardtSep 5, 2024
  3. Junio C HamanoSep 5, 2024
  4. Junio C HamanoSep 5, 2024
  5. Jeff KingSep 6, 2024
  6. Junio C HamanoSep 6, 2024
  7. Junio C HamanoSep 5, 2024
  8. 0/3 sparse-checkout file handle leak fixJeff King, Sep 6, 2024
  9. 1/3 sparse-checkout: consolidate cleanup when writing patternsJeff King, Sep 6, 2024
  10. 2/3 sparse-checkout: check commit_lock_file when writing patternsJeff King, Sep 6, 2024
  11. 3/3 sparse-checkout: use fdopen_lock_file() instead of xfdopen()Jeff King, Sep 6, 2024
  12. Patrick SteinhardtSep 6, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.