From 2a00e594e5c589d05da250eb622273977eb06ad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 20 May 2018 12:42:33 +0200 Subject: config: free resources of `struct config_store_data` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 all 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. In particular, we leak the memory pointed to by the `parsed` and `seen` fields. Introduce and use a helper function `config_store_data_clear()` to plug these leaks. 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. There are two more members of the struct that are candidates for freeing in this new function (`key` and `value_regex`). Those are actually already being taken care of. The next couple of patches will move their freeing into the function we are adding here. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- config.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'config.c') diff --git a/config.c b/config.c index 6f8f1d8c11..b3282f7193 100644 --- a/config.c +++ b/config.c @@ -2333,6 +2333,13 @@ struct config_store_data { unsigned int key_seen:1, section_seen:1, is_keys_section:1; }; +static void config_store_data_clear(struct config_store_data *store) +{ + free(store->parsed); + free(store->seen); + memset(store, 0, sizeof(*store)); +} + static int matches(const char *key, const char *value, const struct config_store_data *store) { @@ -2887,6 +2894,7 @@ out_free: munmap(contents, contents_sz); if (in_fd >= 0) close(in_fd); + config_store_data_clear(&store); return ret; write_err_out: @@ -3127,6 +3135,7 @@ out: rollback_lock_file(&lock); out_no_rollback: free(filename_buf); + config_store_data_clear(&store); return ret; } -- cgit v1.2.3 From 3b82542dff46339a585c7dde3a40bc543f6212f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 20 May 2018 12:42:34 +0200 Subject: config: let `config_store_data_clear()` handle `value_regex` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of duplicating the logic for clearing up `value_regex`, let `config_store_data_clear()` handle that. When `regcomp()` fails, the current code does not call `regfree()`. Make sure we do the same by immediately invalidating `value_regex`. Some implementations are able to handle such an extra `regfree()`-call [1], but from the example in [2], we should not do so. (The language itself in [2] is not super-clear on this.) [1] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html Researched-by: Eric Sunshine Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- config.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index b3282f7193..ac71f3f2e1 100644 --- a/config.c +++ b/config.c @@ -2335,6 +2335,11 @@ struct config_store_data { static void config_store_data_clear(struct config_store_data *store) { + if (store->value_regex != NULL && + store->value_regex != CONFIG_REGEX_NONE) { + regfree(store->value_regex); + free(store->value_regex); + } free(store->parsed); free(store->seen); memset(store, 0, sizeof(*store)); @@ -2722,7 +2727,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, if (regcomp(store.value_regex, value_regex, REG_EXTENDED)) { error("invalid pattern: %s", value_regex); - free(store.value_regex); + FREE_AND_NULL(store.value_regex); ret = CONFIG_INVALID_PATTERN; goto out_free; } @@ -2748,21 +2753,11 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, &store, &opts)) { error("invalid config file %s", config_filename); free(store.key); - if (store.value_regex != NULL && - store.value_regex != CONFIG_REGEX_NONE) { - regfree(store.value_regex); - free(store.value_regex); - } ret = CONFIG_INVALID_FILE; goto out_free; } free(store.key); - if (store.value_regex != NULL && - store.value_regex != CONFIG_REGEX_NONE) { - regfree(store.value_regex); - free(store.value_regex); - } /* if nothing to unset, or too many matches, error out */ if ((store.seen_nr == 0 && value == NULL) || -- cgit v1.2.3 From e7347cb9ba350880e6ccccd6fa6a33cec04c5111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 20 May 2018 12:42:35 +0200 Subject: config: let `config_store_data_clear()` handle `key` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of remembering to free `key` in each code path, let `config_store_data_clear()` handle that. We still need to free it before replacing it, though. Move that freeing closer to the replacing to be safe. Note that in that same part of the code, we can no longer set `key` to the original pointer, but need to `xstrdup()` it. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- config.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index ac71f3f2e1..339a92235d 100644 --- a/config.c +++ b/config.c @@ -2335,6 +2335,7 @@ struct config_store_data { static void config_store_data_clear(struct config_store_data *store) { + free(store->key); if (store->value_regex != NULL && store->value_regex != CONFIG_REGEX_NONE) { regfree(store->value_regex); @@ -2679,7 +2680,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, fd = hold_lock_file_for_update(&lock, config_filename, 0); if (fd < 0) { error_errno("could not lock config file %s", config_filename); - free(store.key); ret = CONFIG_NO_LOCK; goto out_free; } @@ -2689,8 +2689,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, */ in_fd = open(config_filename, O_RDONLY); if ( in_fd < 0 ) { - free(store.key); - if ( ENOENT != errno ) { error_errno("opening %s", config_filename); ret = CONFIG_INVALID_FILE; /* same as "invalid config file" */ @@ -2702,7 +2700,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, goto out_free; } - store.key = (char *)key; + free(store.key); + store.key = xstrdup(key); if (write_section(fd, key, &store) < 0 || write_pair(fd, key, value, &store) < 0) goto write_err_out; @@ -2752,13 +2751,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, config_filename, &store, &opts)) { error("invalid config file %s", config_filename); - free(store.key); ret = CONFIG_INVALID_FILE; goto out_free; } - free(store.key); - /* if nothing to unset, or too many matches, error out */ if ((store.seen_nr == 0 && value == NULL) || (store.seen_nr > 1 && multi_replace == 0)) { -- cgit v1.2.3