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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 14, 2015, 19:54 UTC
Message-ID: <CAPig+cStMdZUwDgSfzT_DgBpF-OMQe-PqTKbSLkzXFHQW8ca=Q@mail.gmail.com>
In-Reply-To: <55CE4511.6050704@cs-ware.de>

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

Show 37 quoted lines

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