@matheusbsbs / matheus-bagliano / commits / 2c8ee1f53c

bundle: dup() output descriptor closer to point-of-use

When writing a bundle to a file, the bundle code actually creates "your.bundle.lock" using our lockfile interface. We feed that output descriptor to a child git-pack-objects via run-command, which has the quirk that it closes the output descriptor in the parent. To avoid confusing the lockfile code (which still thinks the descriptor is valid), we dup() it, and operate on the duplicate. However, this has a confusing side effect: after the dup() but before we call pack-objects, we have _two_ descriptors open to the lockfile. If we call die() during that time, the lockfile code will try to clean up the partially-written file. It knows to close() the file before unlinking, since on some platforms (i.e., Windows) the open file would block the deletion. But it doesn't know about the duplicate descriptor. On Windows, triggering an error at the right part of the code will result in the cleanup failing and the lockfile being left in the filesystem. We can solve this by moving the dup() much closer to start_command(), shrinking the window in which we have the second descriptor open. It's easy to place this in such a way that no die() is possible. We could still die due to a signal in the exact wrong moment, but we already tolerate races there (e.g., a signal could come before we manage to put the file on the cleanup list in the first place). As a bonus, this shields create_bundle() itself from the duplicate-fd trick, and we can simplify its error handling (note that the lock rollback now happens unconditionally, but that's OK; it's a noop if we didn't open the lock in the first place). The included test uses an empty bundle to cause a failure at the right spot in the code, because that's easy to trigger (the other likely errors are write() problems like ENOSPC). Note that it would already pass on non-Windows systems (because they are happy to unlink an already-open file). Based-on-a-patch-by: Gaël Lhez <gael.lhez@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Tested-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Nov 16, 2018 at 04:43 UTC 2c8ee1f53c7048aa391ac810306bf4cb8e15f2bc

2 files changed +24 -21

bundle.c

+18 -21

@@ -243,7 +243,7 @@ out:

243 }

244

245

246 -/* Write the pack data to bundle_fd, then close it if it is > 1. */

246 +/* Write the pack data to bundle_fd */

247 static int write_pack_data(int bundle_fd, struct rev_info *revs)

248 {

249 struct child_process pack_objects = CHILD_PROCESS_INIT;

@@ -256,6 +256,20 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs)

256 pack_objects.in = -1;

257 pack_objects.out = bundle_fd;

258 pack_objects.git_cmd = 1;

259 +

260 + /*

261 + * start_command() will close our descriptor if it's >1. Duplicate it

262 + * to avoid surprising the caller.

263 + */

264 + if (pack_objects.out > 1) {

265 + pack_objects.out = dup(pack_objects.out);

266 + if (pack_objects.out < 0) {

267 + error_errno(_("unable to dup bundle descriptor"));

268 + child_process_clear(&pack_objects);

269 + return -1;

270 + }

271 + }

272 +

273 if (start_command(&pack_objects))

274 return error(_("Could not spawn pack-objects"));

275

@@ -421,21 +435,10 @@ int create_bundle(struct bundle_header *header, const char *path,

435 bundle_to_stdout = !strcmp(path, "-");

436 if (bundle_to_stdout)

437 bundle_fd = 1;

424 - else {

438 + else

439 bundle_fd = hold_lock_file_for_update(&lock, path,

440 LOCK_DIE_ON_ERROR);

441

428 - /*

429 - * write_pack_data() will close the fd passed to it,

430 - * but commit_lock_file() will also try to close the

431 - * lockfile's fd. So make a copy of the file

432 - * descriptor to avoid trying to close it twice.

433 - */

434 - bundle_fd = dup(bundle_fd);

435 - if (bundle_fd < 0)

436 - die_errno("unable to dup file descriptor");

437 - }

438 -

442 /* write signature */

443 write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));

444

@@ -463,10 +466,8 @@ int create_bundle(struct bundle_header *header, const char *path,

466 goto err;

467

468 /* write pack */

466 - if (write_pack_data(bundle_fd, &revs)) {

467 - bundle_fd = -1; /* already closed by the above call */

469 + if (write_pack_data(bundle_fd, &revs))

470 goto err;

469 - }

471

472 if (!bundle_to_stdout) {

473 if (commit_lock_file(&lock))

@@ -474,11 +475,7 @@ int create_bundle(struct bundle_header *header, const char *path,

475 }

476 return 0;

477 err:

477 - if (!bundle_to_stdout) {

478 - if (0 <= bundle_fd)

479 - close(bundle_fd);

480 - rollback_lock_file(&lock);

481 - }

478 + rollback_lock_file(&lock);

479 return -1;

480 }

481

t/t5607-clone-bundle.sh

@@ -71,4 +71,10 @@ test_expect_success 'prerequisites with an empty commit message' '

71 git bundle verify bundle

72 '

74 +test_expect_success 'failed bundle creation does not leave cruft' '

75 + # This fails because the bundle would be empty.

76 + test_must_fail git bundle create fail.bundle master..master &&

77 + test_path_is_missing fail.bundle.lock

78 +'

79 +

80 test_done