Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Mar 30, 2016 at 1:05 PM, Stefan Beller <sbeller@google.com> wrote:
> In successful operation `write_pack_data` will close the `bundle_fd`,
> but when we exit early, we need to take care of the file descriptor
> as well as the lock file ourselves. The lock file may be deleted at the
> end of running the program, but we are in library code, so we should
> not rely on that.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/bundle.c b/bundle.c
> @@ -435,30 +436,40 @@ int create_bundle(struct bundle_header *header, const char *path,
>
>         /* write prerequisites */
>         if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))
> -               return -1;
> +               goto err;
>
>         argc = setup_revisions(argc, argv, &revs, NULL);
>
> -       if (argc > 1)
> -               return error(_("unrecognized argument: %s"), argv[1]);
> +       if (argc > 1) {
> +               ret = error(_("unrecognized argument: %s"), argv[1]);
> +               goto err;
> +       }
>
>         object_array_remove_duplicates(&revs.pending);
>
>         ref_count = write_bundle_refs(bundle_fd, &revs);
>         if (!ref_count)
>                 die(_("Refusing to create empty bundle."));
> -       else if (ref_count < 0)
> -               return -1;
> +       else if (ref_count < 0) {
> +               if (!bundle_to_stdout)
> +                       close(bundle_fd);

Why is this close() here considering that it gets closed by the 'err' path?

> +               goto err;
> +       }
>
>         /* write pack */
>         if (write_pack_data(bundle_fd, &revs))
> -               return -1;
> +               goto err;
>
>         if (!bundle_to_stdout) {
>                 if (commit_lock_file(&lock))
>                         die_errno(_("cannot create '%s'"), path);
>         }
>         return 0;
> +err:
> +       if (!bundle_to_stdout)
> +               close(bundle_fd);
> +       rollback_lock_file(&lock);
> +       return ret;
>  }