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

[PATCH v2 0/3] sparse-checkout file handle leak fix

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

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);
      
Previous: Junio C HamanoNext: Jeff King
Message 8 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.