Re: [PATCH] config: free resources of `struct config_store_data`

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 13, 2018, 08:59 UTC
Message-ID: <CAPig+cThT3VVw75VF0wuF-yR-xbnfNOxhviYGZLAksF8HFuOGw@mail.gmail.com>
In-Reply-To: <20180513082303.19582-1-martin.agren@gmail.com>

On Sun, May 13, 2018 at 4:23 AM Martin Ågren <martin.agren@gmail.com> wrote:

> Commit fee8572c6d (config: avoid using the global variable `store`,
> 2018-04-09) dropped the staticness of a certain struct, instead letting
> the users create an instance on the stack and pass around a pointer.

> We do not free the memory that the struct tracks. When the struct was
> static, the memory would always be reachable. Now that we keep the
> struct on the stack, though, as soon as we return, it goes out of scope
> and we leak the memory it points to.

Show 5 quoted lines

> Introduce and use a small helper function `config_store_data_clear()` to
> plug these leaks. This should be safe. The memory tracked here is config
> parser events. Once the users (`git_config_set_multivar_in_file_gently()`
> and `git_config_copy_or_rename_section_in_file()` at the moment) are
> done, no-one should be holding on to a pointer into this memory.

Show 10 quoted lines

> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> diff --git a/config.c b/config.c
> @@ -2333,6 +2333,13 @@ struct config_store_data {
> +void config_store_data_clear(struct config_store_data *store)
> +{
> +       free(store->parsed);
> +       free(store->seen);
> +       memset(store, 0, sizeof(*store));
> +}

A newcomer to this code (such as myself) might legitimately wonder why store->key and store->value_regex are not also being cleaned up by this function. An examination of the relevant code reveals that those structure members are manually (and perhaps tediously) freed already by git_config_set_multivar_in_file_gently(), however, if config_store_data_clear() was responsible for freeing them, then git_config_set_multivar_in_file_gently() could be made a bit less noisy.

On the other hand, if there's actually a good reason for
  config_store_data_clear() not freeing those members, then perhaps it
deserves an in-code comment explaining why they are exempt.