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

From: Jeff King <peff@peff.net>
Date: Mar 31, 2016, 19:11 UTC
Message-ID: <20160331191151.GA5013@sigill.intra.peff.net>
In-Reply-To: <1459447446-32260-4-git-send-email-sbeller@google.com>

On Thu, Mar 31, 2016 at 11:04:05AM -0700, Stefan Beller wrote:

Show 33 quoted lines

> diff --git a/bundle.c b/bundle.c
> index 506ac49..31ae1da 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -435,12 +435,14 @@ 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) {
> +		error(_("unrecognized argument: %s"), argv[1]);
> +		goto err;
> +	}
>  
>  	object_array_remove_duplicates(&revs.pending);
>  
> @@ -448,17 +450,22 @@ int create_bundle(struct bundle_header *header, const char *path,
>  	if (!ref_count)
>  		die(_("Refusing to create empty bundle."));
>  	else if (ref_count < 0)
> -		return -1;
> +		goto err;
>  
>  	/* write pack */
>  	if (write_pack_data(bundle_fd, &revs))
> -		return -1;
> +		goto err;

Sorry for not noticing this before, but if we assume write_pack_data always closes bundle_fd, even on error (and I think it does), then the close() in the "err" code path is redundant from this goto, right?

I guess it is harmless, as nobody could have opened a new descriptor in the interim, so our close(bundle_fd) will just get EBADF. But I guess we could also do:

  if (write_pack_data(bundle_fd, &revs)) {
	bundle_fd = -1;
	goto err;
  }

or even:

  ret = write_pack_data(bundle_fd, &revs);
  bundle_fd = -1; /* closed by write_pack_data */
  if (ret)
	goto err;

to ensure that nobody (including the non-error code paths) uses it again.

Like I said, I don't think it's buggy in the current code, but it does seem a little fragile.

Show 5 quoted lines

> +err:
> +	if (!bundle_to_stdout)
> +		close(bundle_fd);
> +	rollback_lock_file(&lock);
> +	return -1;

Similarly, I think the rollback_lock_file() call is redundant if bundle_to_stdout is set (because we don't have created a lockfile in the first place). I think this is more explicitly OK, because the lockfile keeps an "am I initialized" flag, but perhaps sticking inside the "if (!bundle_to_stdout)" condition makes it more clear that it's not an error (especially because the "commit_lock_file" call above is inside one).

-Peff