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

From: Philip Oakley <philipoakley@iee.org>
Date: Mar 31, 2016, 19:00 UTC
Message-ID: <9C112693D94545DC917C90299E52A719@PhilipOakley>
In-Reply-To: <1459357518-14913-4-git-send-email-sbeller@google.com>

From: "Stefan Beller" <sbeller@google.com>

Show 8 quoted lines

> 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>

Has this been tested on Windows? I had a similar problem very recently https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ where a bad rev-list-arg would cause the `bundle create` to die, and, on windows, leave the incomplete bundle file locked.

dscho suggested one possible solution for that, but I haven't had any time to try any patches.

Show 73 quoted lines

> ---
> bundle.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 506ac49..fbc8593 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -407,6 +407,7 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>  int bundle_to_stdout;
>  int ref_count = 0;
>  struct rev_info revs;
> + int ret = -1;
>
>  bundle_to_stdout = !strcmp(path, "-");
>  if (bundle_to_stdout)
> @@ -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);
> + 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;
> }
>
> int unbundle(struct bundle_header *header, int bundle_fd, int flags)
> -- 
> 2.8.0.2.gb331331
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>