[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);