@matheusbsbs / matheus-bagliano / commits / e54c347c1c

create_bundle(): duplicate file descriptor to avoid closing it twice

write_pack_data() passes bundle_fd to start_command() to be used as the stdout of pack-objects. But start_command() closes its stdout if it is > 1. This is a problem if bundle_fd is the fd of a lock_file, because commit_lock_file() will also try to close the fd. So the old code suppressed commit_lock_file()'s usual behavior of closing the file descriptor by setting the lock_file object's fd field to -1. But this is not really kosher. Code here shouldn't be mutating fields within the lock_file object. Instead, duplicate the file descriptor before passing it to write_pack_data(). Then that function can close its copy without closing the copy held in the lock_file object. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Michael Haggerty committed Aug 10, 2015 at 11:47 UTC e54c347c1c444c0f37b64b8735c50a66ee0527e9

1 file changed +16 -10

bundle.c

+16 -10

@@ -235,7 +235,9 @@ out:

235 return result;

236 }

237

238 -static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs)

238 +

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

240 +static int write_pack_data(int bundle_fd, struct rev_info *revs)

241 {

242 struct child_process pack_objects = CHILD_PROCESS_INIT;

243 int i;

@@ -250,13 +252,6 @@ static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_inf

252 if (start_command(&pack_objects))

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

254

253 - /*

254 - * start_command closed bundle_fd if it was > 1

255 - * so set the lock fd to -1 so commit_lock_file()

256 - * won't fail trying to close it.

257 - */

258 - lock->fd = -1;

259 -

255 for (i = 0; i < revs->pending.nr; i++) {

256 struct object *object = revs->pending.objects[i].item;

257 if (object->flags & UNINTERESTING)

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

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

412 if (bundle_to_stdout)

413 bundle_fd = 1;

419 - else

414 + else {

415 bundle_fd = hold_lock_file_for_update(&lock, path,

416 LOCK_DIE_ON_ERROR);

417

418 + /*

419 + * write_pack_data() will close the fd passed to it,

420 + * but commit_lock_file() will also try to close the

421 + * lockfile's fd. So make a copy of the file

422 + * descriptor to avoid trying to close it twice.

423 + */

424 + bundle_fd = dup(bundle_fd);

425 + if (bundle_fd < 0)

426 + die_errno("unable to dup file descriptor");

427 + }

428 +

429 /* write signature */

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

431

@@ -445,7 +451,7 @@ int create_bundle(struct bundle_header *header, const char *path,

451 return -1;

452

453 /* write pack */

448 - if (write_pack_data(bundle_fd, &lock, &revs))

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

455 return -1;

456

457 if (!bundle_to_stdout) {