Re: [PATCH] Correctly close config file handle in case of error

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Aug 14, 2015 at 3:44 PM, Sven Strickroth <sven@cs-ware.de> wrote:
> Without this patch there might be open file handle leaks.

Thanks, this looks better. One comment below...

> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> Signed-off-by: Sup Yut Sum <ch3cooli@gmail.com>
> ---
> diff --git a/config.c b/config.c
> index 9fd275f..c06dc2f 100644
> --- a/config.c
> +++ b/config.c
> @@ -2010,6 +2010,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
>                                 error("invalid pattern: %s", value_regex);
>                                 free(store.value_regex);
>                                 ret = CONFIG_INVALID_PATTERN;
> +                               close(in_fd);
>                                 goto out_free;
>                         }
>                 }
> @@ -2034,6 +2035,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
>                                 free(store.value_regex);
>                         }
>                         ret = CONFIG_INVALID_FILE;
> +                       close(in_fd);
>                         goto out_free;
>                 }
>
> @@ -2048,6 +2050,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
>                 if ((store.seen == 0 && value == NULL) ||
>                                 (store.seen > 1 && multi_replace == 0)) {
>                         ret = CONFIG_NOTHING_SET;
> +                       close(in_fd);
>                         goto out_free;
>                 }
>
> @@ -2062,6 +2065,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
>                               config_filename, strerror(errno));
>                         ret = CONFIG_INVALID_FILE;
>                         contents = NULL;
> +                       close(in_fd);
>                         goto out_free;

Each of these cases flows through 'out_free', so an alternate approach
would be to close 'in_fd' there instead. Doing so has the benefit that
it is less likely for future code changes to make the same mistake of
failing to close the file descriptor.

Of course, you'd need to initialize 'in_fd' to some "invalid" value
(such as -1) which 'out_free' can check, as well as setting 'in_fd' to
that invalid value after the legitimate existing close().

    int in_fd = -1;
    ...
    if (whatever_error)
    goto out_free;
    ...
    close(in_fd);
    in_fd = -1;
    ...
    out_free:
    if (in_fd >= 0)
        close(in_fd);
    ...

or something...

>                 }
>                 close(in_fd);