[PATCH v2 0/3] sparse-checkout file handle leak fix
From: Jeff King <hidden>
Date: 2024-09-06 03:45:58
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);