[PATCH v2 02/16] create_bundle(): duplicate file descriptor to avoid closing it twice
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Aug 10, 2015, 09:47 UTC
- Message-ID
- <82dbb49a0212672ed74c928f2b97fa29b7a7c280.1439198011.git.mhagger@alum.mit.edu>
- In-Reply-To
- <cover.1439198011.git.mhagger@alum.mit.edu>
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> --- This is new since v1. I like that it is better decoupled than the old code, but let me know if you think otherwise.
Actually, it seems to me that start_command()'s special case of not closing fd==0 is weird. I suppose that is because fd==0 is used to mean "no redirections" whereas 0 also happens to be the fd for stdin. But I don't want to dig into that now.
bundle.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/bundle.c b/bundle.c index f732c92..b9dacc0 100644 --- a/bundle.c +++ b/bundle.c @@ -235,7 +235,9 @@ out: return result; } -static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs) + +/* Write the pack data to bundle_fd, then close it if it is > 1. */ +static int write_pack_data(int bundle_fd, struct rev_info *revs) { struct child_process pack_objects = CHILD_PROCESS_INIT; int i; @@ -250,13 +252,6 @@ static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_inf if (start_command(&pack_objects)) return error(_("Could not spawn pack-objects")); - /* - * start_command closed bundle_fd if it was > 1 - * so set the lock fd to -1 so commit_lock_file() - * won't fail trying to close it. - */ - lock->fd = -1; - for (i = 0; i < revs->pending.nr; i++) { struct object *object = revs->pending.objects[i].item; if (object->flags & UNINTERESTING) @@ -416,10 +411,21 @@ int create_bundle(struct bundle_header *header, const char *path, bundle_to_stdout = !strcmp(path, "-"); if (bundle_to_stdout) bundle_fd = 1; - else + else { bundle_fd = hold_lock_file_for_update(&lock, path, LOCK_DIE_ON_ERROR); + /* + * write_pack_data() will close the fd passed to it, + * but commit_lock_file() will also try to close the + * lockfile's fd. So make a copy of the file + * descriptor to avoid trying to close it twice. + */ + bundle_fd = dup(bundle_fd); + if (bundle_fd < 0) + die_errno("unable to dup file descriptor"); + } + /* write signature */ write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); @@ -445,7 +451,7 @@ int create_bundle(struct bundle_header *header, const char *path, return -1; /* write pack */ - if (write_pack_data(bundle_fd, &lock, &revs)) + if (write_pack_data(bundle_fd, &revs)) return -1; if (!bundle_to_stdout) {
-- 2.5.0