{"thread":{"id":"48478","subject":"[PATCH] config: free resources of `struct config_store_data`","startedAt":"2018-05-13T08:23:28Z","lastAt":"2018-05-23T07:03:51Z","messageCount":18,"participants":["Martin Ågren","Eric Sunshine","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"347531","messageId":"20180513082303.19582-1-martin.agren@gmail.com","threadId":"48478","inReplyTo":null,"subject":"[PATCH] config: free resources of `struct config_store_data`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-13T08:23:03Z","receivedAt":"2018-05-13T08:23:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit fee8572c6d (config: avoid using the global variable `store`,\n2018-04-09) dropped the staticness of a certain struct, instead letting\nthe users create an instance on the stack and pass around a pointer.\n\nWe do not free the memory that the struct tracks. When the struct was\nstatic, the memory would always be reachable. Now that we keep the\nstruct on the stack, though, as soon as we return, it goes out of scope\nand we leak the memory it points to.\n\nIntroduce and use a small helper function `config_store_data_clear()` to\nplug these leaks. This should be safe. The memory tracked here is config\nparser events. Once the users (`git_config_set_multivar_in_file_gently()`\nand `git_config_copy_or_rename_section_in_file()` at the moment) are\ndone, no-one should be holding on to a pointer into this memory.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n config.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 6f8f1d8c11..83d7d0851a 100644\n--- a/config.c\n+++ b/config.c\n@@ -2333,6 +2333,13 @@ struct config_store_data {\n \tunsigned int key_seen:1, section_seen:1, is_keys_section:1;\n };\n \n+void config_store_data_clear(struct config_store_data *store)\n+{\n+\tfree(store->parsed);\n+\tfree(store->seen);\n+\tmemset(store, 0, sizeof(*store));\n+}\n+\n static int matches(const char *key, const char *value,\n \t\t   const struct config_store_data *store)\n {\n@@ -2887,6 +2894,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tmunmap(contents, contents_sz);\n \tif (in_fd >= 0)\n \t\tclose(in_fd);\n+\tconfig_store_data_clear(&store);\n \treturn ret;\n \n write_err_out:\n@@ -3127,6 +3135,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \trollback_lock_file(&lock);\n out_no_rollback:\n \tfree(filename_buf);\n+\tconfig_store_data_clear(&store);\n \treturn ret;\n }\n \n-- \n2.17.0.583.g9a75a153ac\n\n"},{"id":"347532","messageId":"CAPig+cThT3VVw75VF0wuF-yR-xbnfNOxhviYGZLAksF8HFuOGw@mail.gmail.com","threadId":"48478","inReplyTo":"20180513082303.19582-1-martin.agren@gmail.com","subject":"Re: [PATCH] config: free resources of `struct config_store_data`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-13T08:59:13Z","receivedAt":"2018-05-13T08:59:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 13, 2018 at 4:23 AM Martin Ågren <martin.agren@gmail.com> wrote:\n> Commit fee8572c6d (config: avoid using the global variable `store`,\n> 2018-04-09) dropped the staticness of a certain struct, instead letting\n> the users create an instance on the stack and pass around a pointer.\n\n> We do not free the memory that the struct tracks. When the struct was\n> static, the memory would always be reachable. Now that we keep the\n> struct on the stack, though, as soon as we return, it goes out of scope\n> and we leak the memory it points to.\n\n> Introduce and use a small helper function `config_store_data_clear()` to\n> plug these leaks. This should be safe. The memory tracked here is config\n> parser events. Once the users (`git_config_set_multivar_in_file_gently()`\n> and `git_config_copy_or_rename_section_in_file()` at the moment) are\n> done, no-one should be holding on to a pointer into this memory.\n\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n> diff --git a/config.c b/config.c\n> @@ -2333,6 +2333,13 @@ struct config_store_data {\n> +void config_store_data_clear(struct config_store_data *store)\n> +{\n> +       free(store->parsed);\n> +       free(store->seen);\n> +       memset(store, 0, sizeof(*store));\n> +}\n\nA newcomer to this code (such as myself) might legitimately wonder why\nstore->key and store->value_regex are not also being cleaned up by this\nfunction. An examination of the relevant code reveals that those structure\nmembers are manually (and perhaps tediously) freed already by\ngit_config_set_multivar_in_file_gently(), however, if\nconfig_store_data_clear() was responsible for freeing them, then\ngit_config_set_multivar_in_file_gently() could be made a bit less noisy.\n\nOn the other hand, if there's actually a good reason for\n  config_store_data_clear() not freeing those members, then perhaps it\ndeserves an in-code comment explaining why they are exempt.\n"},{"id":"347534","messageId":"cover.1526204440.git.martin.agren@gmail.com","threadId":"48478","inReplyTo":"CAPig+cThT3VVw75VF0wuF-yR-xbnfNOxhviYGZLAksF8HFuOGw@mail.gmail.com","subject":"Re: [PATCH] config: free resources of `struct config_store_data`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-13T09:58:36Z","receivedAt":"2018-05-13T09:58:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 13 May 2018 at 10:59, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Sun, May 13, 2018 at 4:23 AM Martin Ågren <martin.agren@gmail.com> wrote:\n>\n>> Introduce and use a small helper function `config_store_data_clear()` to\n>> plug these leaks. This should be safe. The memory tracked here is config\n>> parser events. Once the users (`git_config_set_multivar_in_file_gently()`\n>> and `git_config_copy_or_rename_section_in_file()` at the moment) are\n>> done, no-one should be holding on to a pointer into this memory.\n>\n> A newcomer to this code (such as myself) might legitimately wonder why\n> store->key and store->value_regex are not also being cleaned up by this\n\nGood point. I was only concerned by the members that no-one took\nresponsibility for.\n\n> function. An examination of the relevant code reveals that those structure\n> members are manually (and perhaps tediously) freed already by\n> git_config_set_multivar_in_file_gently(), however, if\n> config_store_data_clear() was responsible for freeing them, then\n> git_config_set_multivar_in_file_gently() could be made a bit less noisy.\n\nYes, that's true.\n\n> On the other hand, if there's actually a good reason for\n>   config_store_data_clear() not freeing those members, then perhaps it\n> deserves an in-code comment explaining why they are exempt.\n\nNot any good reason that I can think of, other than \"history\". But to be\nclear, a day ago I was as much of a newcomer in this part of the code as\nyou are. Johannes is the one who might have the most up-to-date\nunderstanding of this.\n\nHow about the following two patches as patches 2/3 and 3/3? I would also\nneed to mention in the commit message of this patch (1/3) that the\nfunction will soon learn to clean up more members.\n\nI could of course squash the three patches into one, but there is some\nminor trickery involved, like we can't reuse a pointer in one place, but\nneed to xstrdup it.\n\nThank you for your comments. I'd be very interested in your thoughts on\nthis.\n\nMartin\n\nMartin Ågren (2):\n  config: let `config_store_data_clear()` handle `value_regex`\n  config: let `config_store_data_clear()` handle `key`\n\n config.c | 26 ++++++++------------------\n 1 file changed, 8 insertions(+), 18 deletions(-)\n\n-- \n2.17.0.583.g9a75a153ac\n\n"},{"id":"347535","messageId":"3f64a4e24a83a9041c79c8b302207af7687dbb21.1526204440.git.martin.agren@gmail.com","threadId":"48478","inReplyTo":"cover.1526204440.git.martin.agren@gmail.com","subject":"[PATCH 2/1] config: let `config_store_data_clear()` handle `value_regex`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-13T09:58:37Z","receivedAt":"2018-05-13T09:59:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of carefully clearing up `value_regex` in each code path, let\n`config_store_data_clear()` handle that.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\nI *think* that it should be ok to `regfree()` after `regcomp()` failed,\nbut I'll need to look into that some more (and say something about it in\nthe commit message).\n\n config.c | 16 +++++-----------\n 1 file changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 83d7d0851a..2e3c6c94e9 100644\n--- a/config.c\n+++ b/config.c\n@@ -2335,6 +2335,11 @@ struct config_store_data {\n \n void config_store_data_clear(struct config_store_data *store)\n {\n+\tif (store->value_regex != NULL &&\n+\t    store->value_regex != CONFIG_REGEX_NONE) {\n+\t\tregfree(store->value_regex);\n+\t\tfree(store->value_regex);\n+\t}\n \tfree(store->parsed);\n \tfree(store->seen);\n \tmemset(store, 0, sizeof(*store));\n@@ -2722,7 +2727,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\tif (regcomp(store.value_regex, value_regex,\n \t\t\t\t\tREG_EXTENDED)) {\n \t\t\t\terror(\"invalid pattern: %s\", value_regex);\n-\t\t\t\tfree(store.value_regex);\n \t\t\t\tret = CONFIG_INVALID_PATTERN;\n \t\t\t\tgoto out_free;\n \t\t\t}\n@@ -2748,21 +2752,11 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\t\t\t      &store, &opts)) {\n \t\t\terror(\"invalid config file %s\", config_filename);\n \t\t\tfree(store.key);\n-\t\t\tif (store.value_regex != NULL &&\n-\t\t\t    store.value_regex != CONFIG_REGEX_NONE) {\n-\t\t\t\tregfree(store.value_regex);\n-\t\t\t\tfree(store.value_regex);\n-\t\t\t}\n \t\t\tret = CONFIG_INVALID_FILE;\n \t\t\tgoto out_free;\n \t\t}\n \n \t\tfree(store.key);\n-\t\tif (store.value_regex != NULL &&\n-\t\t    store.value_regex != CONFIG_REGEX_NONE) {\n-\t\t\tregfree(store.value_regex);\n-\t\t\tfree(store.value_regex);\n-\t\t}\n \n \t\t/* if nothing to unset, or too many matches, error out */\n \t\tif ((store.seen_nr == 0 && value == NULL) ||\n-- \n2.17.0.583.g9a75a153ac\n\n"},{"id":"347536","messageId":"8873a578e1dbc0d1c6566e1a68b7b0a7741b6b0d.1526204440.git.martin.agren@gmail.com","threadId":"48478","inReplyTo":"cover.1526204440.git.martin.agren@gmail.com","subject":"[PATCH 3/1] config: let `config_store_data_clear()` handle `key`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-13T09:58:38Z","receivedAt":"2018-05-13T09:59:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of carefully clearing up `key` in each code path, let\n`config_store_data_clear()` handle that.\n\nWe still need to free it before replacing it, though. Move that freeing\ncloser to the replacing to be safe. Note that in that same part of the\ncode, we can no longer set `key` to the original pointer, but need to\n`xstrdup()` it.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n config.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2e3c6c94e9..963edacf10 100644\n--- a/config.c\n+++ b/config.c\n@@ -2335,6 +2335,7 @@ struct config_store_data {\n \n void config_store_data_clear(struct config_store_data *store)\n {\n+\tfree(store->key);\n \tif (store->value_regex != NULL &&\n \t    store->value_regex != CONFIG_REGEX_NONE) {\n \t\tregfree(store->value_regex);\n@@ -2679,7 +2680,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n \tif (fd < 0) {\n \t\terror_errno(\"could not lock config file %s\", config_filename);\n-\t\tfree(store.key);\n \t\tret = CONFIG_NO_LOCK;\n \t\tgoto out_free;\n \t}\n@@ -2689,8 +2689,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t */\n \tin_fd = open(config_filename, O_RDONLY);\n \tif ( in_fd < 0 ) {\n-\t\tfree(store.key);\n-\n \t\tif ( ENOENT != errno ) {\n \t\t\terror_errno(\"opening %s\", config_filename);\n \t\t\tret = CONFIG_INVALID_FILE; /* same as \"invalid config file\" */\n@@ -2702,7 +2700,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\tgoto out_free;\n \t\t}\n \n-\t\tstore.key = (char *)key;\n+\t\tfree(store.key);\n+\t\tstore.key = xstrdup(key);\n \t\tif (write_section(fd, key, &store) < 0 ||\n \t\t    write_pair(fd, key, value, &store) < 0)\n \t\t\tgoto write_err_out;\n@@ -2751,13 +2750,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\t\t\t      config_filename,\n \t\t\t\t\t\t      &store, &opts)) {\n \t\t\terror(\"invalid config file %s\", config_filename);\n-\t\t\tfree(store.key);\n \t\t\tret = CONFIG_INVALID_FILE;\n \t\t\tgoto out_free;\n \t\t}\n \n-\t\tfree(store.key);\n-\n \t\t/* if nothing to unset, or too many matches, error out */\n \t\tif ((store.seen_nr == 0 && value == NULL) ||\n \t\t    (store.seen_nr > 1 && multi_replace == 0)) {\n-- \n2.17.0.583.g9a75a153ac\n\n"},{"id":"347552","messageId":"CAN0heSrFYyGiVwJYceC--wB88=taSis=KrdGpSfiGk1orC754A@mail.gmail.com","threadId":"48478","inReplyTo":"20180513082303.19582-1-martin.agren@gmail.com","subject":"Re: [PATCH] config: free resources of `struct config_store_data`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-13T18:40:30Z","receivedAt":"2018-05-13T18:40:35Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 13 May 2018 at 10:23, Martin Ågren <martin.agren@gmail.com> wrote:\n\n> +void config_store_data_clear(struct config_store_data *store)\n\nI will do s/void/static void/ here...\n"},{"id":"347565","messageId":"CAPig+cT1eejizU26mPE+tN+eZR4Wg0jw1DbmShgqRu69EYQutQ@mail.gmail.com","threadId":"48478","inReplyTo":"cover.1526204440.git.martin.agren@gmail.com","subject":"Re: [PATCH] config: free resources of `struct config_store_data`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-14T03:03:39Z","receivedAt":"2018-05-14T03:03:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 13, 2018 at 5:58 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n> On 13 May 2018 at 10:59, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Sun, May 13, 2018 at 4:23 AM Martin Ågren <martin.agren@gmail.com> wrote:\n>>> Introduce and use a small helper function `config_store_data_clear()` to\n>>> plug these leaks. This should be safe. The memory tracked here is config\n>>> parser events. Once the users (`git_config_set_multivar_in_file_gently()`\n>>> and `git_config_copy_or_rename_section_in_file()` at the moment) are\n>>> done, no-one should be holding on to a pointer into this memory.\n>>\n>> A newcomer to this code (such as myself) might legitimately wonder why\n>> store->key and store->value_regex are not also being cleaned up by this\n>> function. An examination of the relevant code reveals that those structure\n>> members are manually (and perhaps tediously) freed already by\n>> git_config_set_multivar_in_file_gently(), however, if\n>> config_store_data_clear() was responsible for freeing them, then\n>> git_config_set_multivar_in_file_gently() could be made a bit less noisy.\n>\n> How about the following two patches as patches 2/3 and 3/3? I would also\n> need to mention in the commit message of this patch (1/3) that the\n> function will soon learn to clean up more members.\n>\n> I could of course squash the three patches into one, but there is some\n> minor trickery involved, like we can't reuse a pointer in one place, but\n> need to xstrdup it.\n>\n> Thank you for your comments. I'd be very interested in your thoughts on\n> this.\n\nYep, making this a multi-part patch series and updating the commit\nmessage of the patch which introduces config_store_data_clear(), as\nyou suggest, makes sense. The patch series could be organized\ndifferently -- such as first moving freeing of 'value_regex' into new\nconfig_store_data_clear(), then freeing additional fields in later\npatches -- but I don't think it matters much in practice, so the\ncurrent organization is likely good enough.\n"},{"id":"347566","messageId":"CAPig+cQaJmsJe=E6HdBw0s6eN-qW0OCezBacRcQ+UnJYMbODsw@mail.gmail.com","threadId":"48478","inReplyTo":"3f64a4e24a83a9041c79c8b302207af7687dbb21.1526204440.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/1] config: let `config_store_data_clear()` handle `value_regex`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-14T03:05:54Z","receivedAt":"2018-05-14T03:05:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 13, 2018 at 5:58 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n> Instead of carefully clearing up `value_regex` in each code path, let\n> `config_store_data_clear()` handle that.\n>\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n> I *think* that it should be ok to `regfree()` after `regcomp()` failed,\n> but I'll need to look into that some more (and say something about it in\n> the commit message).\n\nMy research (for instance [1,2]) seems to indicate that it would be\nbetter to avoid regfree() upon failed regcomp(), even though such a\nsituation is handled sanely in some implementations.\n\n[1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html\n[2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html\n"},{"id":"348090","messageId":"cover.1526812503.git.martin.agren@gmail.com","threadId":"48478","inReplyTo":"CAPig+cT1eejizU26mPE+tN+eZR4Wg0jw1DbmShgqRu69EYQutQ@mail.gmail.com","subject":"[PATCH v2 0/3] config: free resources of `struct config_store_data`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-20T10:42:32Z","receivedAt":"2018-05-20T10:43:35Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 14 May 2018 at 05:03, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Sun, May 13, 2018 at 5:58 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n>>\n>> How about the following two patches as patches 2/3 and 3/3? I would also\n>> need to mention in the commit message of this patch (1/3) that the\n>> function will soon learn to clean up more members.\n>>\n>\n> Yep, making this a multi-part patch series and updating the commit\n> message of the patch which introduces config_store_data_clear(), as\n> you suggest, makes sense. The patch series could be organized\n> differently -- such as first moving freeing of 'value_regex' into new\n> config_store_data_clear(), then freeing additional fields in later\n> patches -- but I don't think it matters much in practice, so the\n> current organization is likely good enough.\n\nI tried such a re-ordering but wasn't entirely happy about the result\n(maybe I didn't try hard enough), so here are these patches again, as a\nproper series and with improved commit messages.\n\nMartin\n\nMartin Ågren (3):\n  config: free resources of `struct config_store_data`\n  config: let `config_store_data_clear()` handle `value_regex`\n  config: let `config_store_data_clear()` handle `key`\n\n config.c | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\n-- \n2.17.0.840.g5d83f92caf\n\n"},{"id":"348091","messageId":"ecae89b852bb98bab1cc90517bfcabfbe6eeafe8.1526812503.git.martin.agren@gmail.com","threadId":"48478","inReplyTo":"cover.1526812503.git.martin.agren@gmail.com","subject":"[PATCH v2 1/3] config: free resources of `struct config_store_data`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-20T10:42:33Z","receivedAt":"2018-05-20T10:43:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit fee8572c6d (config: avoid using the global variable `store`,\n2018-04-09) dropped the staticness of a certain struct, instead letting\nthe users create an instance on the stack and pass around a pointer.\n\nWe do not free all the memory that the struct tracks. When the struct\nwas static, the memory would always be reachable. Now that we keep the\nstruct on the stack, though, as soon as we return, it goes out of scope\nand we leak the memory it points to. In particular, we leak the memory\npointed to by the `parsed` and `seen` fields.\n\nIntroduce and use a helper function `config_store_data_clear()` to plug\nthese leaks. The memory tracked here is config parser events. Once the\nusers (`git_config_set_multivar_in_file_gently()` and\n`git_config_copy_or_rename_section_in_file()` at the moment) are done,\nno-one should be holding on to a pointer into this memory.\n\nThere are two more members of the struct that are candidates for freeing\nin this new function (`key` and `value_regex`). Those are actually\nalready being taken care of. The next couple of patches will move their\nfreeing into the function we are adding here.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n config.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 6f8f1d8c11..b3282f7193 100644\n--- a/config.c\n+++ b/config.c\n@@ -2333,6 +2333,13 @@ struct config_store_data {\n \tunsigned int key_seen:1, section_seen:1, is_keys_section:1;\n };\n \n+static void config_store_data_clear(struct config_store_data *store)\n+{\n+\tfree(store->parsed);\n+\tfree(store->seen);\n+\tmemset(store, 0, sizeof(*store));\n+}\n+\n static int matches(const char *key, const char *value,\n \t\t   const struct config_store_data *store)\n {\n@@ -2887,6 +2894,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tmunmap(contents, contents_sz);\n \tif (in_fd >= 0)\n \t\tclose(in_fd);\n+\tconfig_store_data_clear(&store);\n \treturn ret;\n \n write_err_out:\n@@ -3127,6 +3135,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \trollback_lock_file(&lock);\n out_no_rollback:\n \tfree(filename_buf);\n+\tconfig_store_data_clear(&store);\n \treturn ret;\n }\n \n-- \n2.17.0.840.g5d83f92caf\n\n"},{"id":"348092","messageId":"ca3af1e24176e0a20c5753e4541299747056be47.1526812503.git.martin.agren@gmail.com","threadId":"48478","inReplyTo":"cover.1526812503.git.martin.agren@gmail.com","subject":"[PATCH v2 2/3] config: let `config_store_data_clear()` handle `value_regex`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-20T10:42:34Z","receivedAt":"2018-05-20T10:43:44Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of duplicating the logic for clearing up `value_regex`, let\n`config_store_data_clear()` handle that.\n\nWhen `regcomp()` fails, the current code does not call `regfree()`. Make\nsure we do the same by immediately invalidating `value_regex`. Some\nimplementations are able to handle such an extra `regfree()`-call [1],\nbut from the example in [2], we should not do so. (The language itself\nin [2] is not super-clear on this.)\n\n[1] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html\n\n[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html\n\nResearched-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n config.c | 17 ++++++-----------\n 1 file changed, 6 insertions(+), 11 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b3282f7193..ac71f3f2e1 100644\n--- a/config.c\n+++ b/config.c\n@@ -2335,6 +2335,11 @@ struct config_store_data {\n \n static void config_store_data_clear(struct config_store_data *store)\n {\n+\tif (store->value_regex != NULL &&\n+\t    store->value_regex != CONFIG_REGEX_NONE) {\n+\t\tregfree(store->value_regex);\n+\t\tfree(store->value_regex);\n+\t}\n \tfree(store->parsed);\n \tfree(store->seen);\n \tmemset(store, 0, sizeof(*store));\n@@ -2722,7 +2727,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\tif (regcomp(store.value_regex, value_regex,\n \t\t\t\t\tREG_EXTENDED)) {\n \t\t\t\terror(\"invalid pattern: %s\", value_regex);\n-\t\t\t\tfree(store.value_regex);\n+\t\t\t\tFREE_AND_NULL(store.value_regex);\n \t\t\t\tret = CONFIG_INVALID_PATTERN;\n \t\t\t\tgoto out_free;\n \t\t\t}\n@@ -2748,21 +2753,11 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\t\t\t      &store, &opts)) {\n \t\t\terror(\"invalid config file %s\", config_filename);\n \t\t\tfree(store.key);\n-\t\t\tif (store.value_regex != NULL &&\n-\t\t\t    store.value_regex != CONFIG_REGEX_NONE) {\n-\t\t\t\tregfree(store.value_regex);\n-\t\t\t\tfree(store.value_regex);\n-\t\t\t}\n \t\t\tret = CONFIG_INVALID_FILE;\n \t\t\tgoto out_free;\n \t\t}\n \n \t\tfree(store.key);\n-\t\tif (store.value_regex != NULL &&\n-\t\t    store.value_regex != CONFIG_REGEX_NONE) {\n-\t\t\tregfree(store.value_regex);\n-\t\t\tfree(store.value_regex);\n-\t\t}\n \n \t\t/* if nothing to unset, or too many matches, error out */\n \t\tif ((store.seen_nr == 0 && value == NULL) ||\n-- \n2.17.0.840.g5d83f92caf\n\n"},{"id":"348093","messageId":"e71b2abd7298de0123a6222186a8faa3fadf50c0.1526812503.git.martin.agren@gmail.com","threadId":"48478","inReplyTo":"cover.1526812503.git.martin.agren@gmail.com","subject":"[PATCH v2 3/3] config: let `config_store_data_clear()` handle `key`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-20T10:42:35Z","receivedAt":"2018-05-20T10:43:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of remembering to free `key` in each code path, let\n`config_store_data_clear()` handle that.\n\nWe still need to free it before replacing it, though. Move that freeing\ncloser to the replacing to be safe. Note that in that same part of the\ncode, we can no longer set `key` to the original pointer, but need to\n`xstrdup()` it.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n config.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex ac71f3f2e1..339a92235d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2335,6 +2335,7 @@ struct config_store_data {\n \n static void config_store_data_clear(struct config_store_data *store)\n {\n+\tfree(store->key);\n \tif (store->value_regex != NULL &&\n \t    store->value_regex != CONFIG_REGEX_NONE) {\n \t\tregfree(store->value_regex);\n@@ -2679,7 +2680,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n \tif (fd < 0) {\n \t\terror_errno(\"could not lock config file %s\", config_filename);\n-\t\tfree(store.key);\n \t\tret = CONFIG_NO_LOCK;\n \t\tgoto out_free;\n \t}\n@@ -2689,8 +2689,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t */\n \tin_fd = open(config_filename, O_RDONLY);\n \tif ( in_fd < 0 ) {\n-\t\tfree(store.key);\n-\n \t\tif ( ENOENT != errno ) {\n \t\t\terror_errno(\"opening %s\", config_filename);\n \t\t\tret = CONFIG_INVALID_FILE; /* same as \"invalid config file\" */\n@@ -2702,7 +2700,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\tgoto out_free;\n \t\t}\n \n-\t\tstore.key = (char *)key;\n+\t\tfree(store.key);\n+\t\tstore.key = xstrdup(key);\n \t\tif (write_section(fd, key, &store) < 0 ||\n \t\t    write_pair(fd, key, value, &store) < 0)\n \t\t\tgoto write_err_out;\n@@ -2752,13 +2751,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\t\t\t      config_filename,\n \t\t\t\t\t\t      &store, &opts)) {\n \t\t\terror(\"invalid config file %s\", config_filename);\n-\t\t\tfree(store.key);\n \t\t\tret = CONFIG_INVALID_FILE;\n \t\t\tgoto out_free;\n \t\t}\n \n-\t\tfree(store.key);\n-\n \t\t/* if nothing to unset, or too many matches, error out */\n \t\tif ((store.seen_nr == 0 && value == NULL) ||\n \t\t    (store.seen_nr > 1 && multi_replace == 0)) {\n-- \n2.17.0.840.g5d83f92caf\n\n"},{"id":"348094","messageId":"20180520105032.9464-1-martin.agren@gmail.com","threadId":"48478","inReplyTo":"CAPig+cQaJmsJe=E6HdBw0s6eN-qW0OCezBacRcQ+UnJYMbODsw@mail.gmail.com","subject":"[PATCH] regex: do not call `regfree()` if compilation fails","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-20T10:50:32Z","receivedAt":"2018-05-20T10:51:06Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"It is apparently undefined behavior to call `regfree()` on a regex where\n`regcomp()` failed. The language in [1] is a bit muddy, at least to me,\nbut the clearest hint is this (`preg` is the `regex_t *`):\n\n    Upon successful completion, the regcomp() function shall return 0.\n    Otherwise, it shall return an integer value indicating an error as\n    described in <regex.h>, and the content of preg is undefined.\n\nFunnily enough, there is also the `regerror()` function which should be\ngiven a pointer to such a \"failed\" `regex_t` -- the content of which\nwould supposedly be undefined -- and which may investigate it to come up\nwith a detailed error message.\n\nIn any case, the example in that document shows how `regfree()` is not\ncalled after `regcomp()` fails.\n\nWe have quite a few users of this API and most get this right. These\nthree users do not.\n\nSeveral implementations can handle this just fine [2] and these code paths\nsupposedly have not wreaked havoc or we'd have heard about it. (These\nare all in code paths where git got bad input and is just about to die\nanyway.) But let's just avoid the issue altogether.\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html\n\n[2] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html\n\nResearched-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-byi Martin Ågren <martin.agren@gmail.com>\n---\n\nOn 14 May 2018 at 05:05, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> My research (for instance [1,2]) seems to indicate that it would be\n> better to avoid regfree() upon failed regcomp(), even though such a\n> situation is handled sanely in some implementations.\n>\n> [1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html\n> [2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html\n\nThank you for researching this. I think it would make sense to get rid\nof the few places we have where we get this wrong (if our understanding\nof this being undefined is right).\n\n diffcore-pickaxe.c | 1 -\n grep.c             | 2 --\n 2 files changed, 3 deletions(-)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 239ce5122b..800a899c86 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)\n \t\t/* The POSIX.2 people are surely sick */\n \t\tchar errbuf[1024];\n \t\tregerror(err, regex, errbuf, 1024);\n-\t\tregfree(regex);\n \t\tdie(\"invalid regex: %s\", errbuf);\n \t}\n }\ndiff --git a/grep.c b/grep.c\nindex 65b90c10a3..5e4f3f9a9d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -636,7 +636,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n \tif (err) {\n \t\tchar errbuf[1024];\n \t\tregerror(err, &p->regexp, errbuf, sizeof(errbuf));\n-\t\tregfree(&p->regexp);\n \t\tcompile_regexp_failed(p, errbuf);\n \t}\n }\n@@ -701,7 +700,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n \tif (err) {\n \t\tchar errbuf[1024];\n \t\tregerror(err, &p->regexp, errbuf, 1024);\n-\t\tregfree(&p->regexp);\n \t\tcompile_regexp_failed(p, errbuf);\n \t}\n }\n-- \n2.17.0.840.g5d83f92caf\n\n"},{"id":"348237","messageId":"CAGZ79kZotwAFauTkCJ6YZ_C-MuaQpNaaS8LCniL_Or=_ccfC4w@mail.gmail.com","threadId":"48478","inReplyTo":"20180520105032.9464-1-martin.agren@gmail.com","subject":"Re: [PATCH] regex: do not call `regfree()` if compilation fails","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-21T18:43:03Z","receivedAt":"2018-05-21T18:43:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, May 20, 2018 at 3:50 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n> It is apparently undefined behavior to call `regfree()` on a regex where\n> `regcomp()` failed. The language in [1] is a bit muddy, at least to me,\n> but the clearest hint is this (`preg` is the `regex_t *`):\n>\n>     Upon successful completion, the regcomp() function shall return 0.\n>     Otherwise, it shall return an integer value indicating an error as\n>     described in <regex.h>, and the content of preg is undefined.\n>\n> Funnily enough, there is also the `regerror()` function which should be\n> given a pointer to such a \"failed\" `regex_t` -- the content of which\n> would supposedly be undefined -- and which may investigate it to come up\n> with a detailed error message.\n>\n> In any case, the example in that document shows how `regfree()` is not\n> called after `regcomp()` fails.\n>\n> We have quite a few users of this API and most get this right. These\n> three users do not.\n>\n> Several implementations can handle this just fine [2] and these code paths\n> supposedly have not wreaked havoc or we'd have heard about it. (These\n> are all in code paths where git got bad input and is just about to die\n> anyway.) But let's just avoid the issue altogether.\n>\n> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html\n>\n> [2] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html\n>\n> Researched-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-byi Martin Ågren <martin.agren@gmail.com>\n> ---\n>\n> On 14 May 2018 at 05:05, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> My research (for instance [1,2]) seems to indicate that it would be\n>> better to avoid regfree() upon failed regcomp(), even though such a\n>> situation is handled sanely in some implementations.\n>>\n>> [1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html\n>> [2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html\n>\n> Thank you for researching this. I think it would make sense to get rid\n> of the few places we have where we get this wrong (if our understanding\n> of this being undefined is right).\n>\n>  diffcore-pickaxe.c | 1 -\n>  grep.c             | 2 --\n>  2 files changed, 3 deletions(-)\n>\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index 239ce5122b..800a899c86 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)\n>                 /* The POSIX.2 people are surely sick */\n>                 char errbuf[1024];\n>                 regerror(err, regex, errbuf, 1024);\n> -               regfree(regex);\n\nWhile the commit message is very clear why we supposedly introduce a leak here,\nit is hard to be found from the source code (as we only delete code\nthere, so digging\nfor history is not obvious), so maybe\n\n     /* regfree(regex) is invalid here */\n\ninstead?\n\n>                 die(\"invalid regex: %s\", errbuf);\n>         }\n>  }\n> diff --git a/grep.c b/grep.c\n> index 65b90c10a3..5e4f3f9a9d 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -636,7 +636,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)\n>         if (err) {\n>                 char errbuf[1024];\n>                 regerror(err, &p->regexp, errbuf, sizeof(errbuf));\n> -               regfree(&p->regexp);\n>                 compile_regexp_failed(p, errbuf);\n>         }\n>  }\n> @@ -701,7 +700,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n>         if (err) {\n>                 char errbuf[1024];\n>                 regerror(err, &p->regexp, errbuf, 1024);\n> -               regfree(&p->regexp);\n>                 compile_regexp_failed(p, errbuf);\n>         }\n>  }\n> --\n> 2.17.0.840.g5d83f92caf\n>\n"},{"id":"348279","messageId":"CAPig+cQevUoYJOisFt9BDhLP1_SjPqX+2hmFKDPcBvAh2D7ksA@mail.gmail.com","threadId":"48478","inReplyTo":"CAGZ79kZotwAFauTkCJ6YZ_C-MuaQpNaaS8LCniL_Or=_ccfC4w@mail.gmail.com","subject":"Re: [PATCH] regex: do not call `regfree()` if compilation fails","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-22T02:20:00Z","receivedAt":"2018-05-22T02:20:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 21, 2018 at 2:43 PM, Stefan Beller <sbeller@google.com> wrote:\n> On Sun, May 20, 2018 at 3:50 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n>> It is apparently undefined behavior to call `regfree()` on a regex where\n>> `regcomp()` failed. [...]\n>>\n>> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n>> @@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)\n>>                 /* The POSIX.2 people are surely sick */\n>>                 char errbuf[1024];\n>>                 regerror(err, regex, errbuf, 1024);\n>> -               regfree(regex);\n>>                 die(\"invalid regex: %s\", errbuf);\n>\n> While the commit message is very clear why we supposedly introduce a leak here,\n> it is hard to be found from the source code (as we only delete code\n> there, so digging\n> for history is not obvious), so maybe\n>\n>      /* regfree(regex) is invalid here */\n>\n> instead?\n\nThe commit message doesn't say that we are supposedly introducing a\nleak (and, indeed, no resources should have been allocated to the\n'regex' upon failed compile). It's saying that removing this call\npotentially avoids a crash under some implementations.\n\nGiven that the very next line is die(), and that the function name has\n\"_or_die\" in it, I'm not sure that an in-code comment about regfree()\nbeing invalid upon failed compile would be all that helpful; indeed,\nit could be confusing, causing the reader to wonder why that is\nsignificant if we're just dying anyhow. I find that the patch, as is,\nclarifies rather than muddles the situation.\n"},{"id":"348294","messageId":"CAN0heSr1BzF2xZ=rRHUDOv3R7dLSHSJzK-QpAc11ARzkXC8UDQ@mail.gmail.com","threadId":"48478","inReplyTo":"CAPig+cQevUoYJOisFt9BDhLP1_SjPqX+2hmFKDPcBvAh2D7ksA@mail.gmail.com","subject":"Re: [PATCH] regex: do not call `regfree()` if compilation fails","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-22T11:00:47Z","receivedAt":"2018-05-22T11:00:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 22 May 2018 at 04:20, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, May 21, 2018 at 2:43 PM, Stefan Beller <sbeller@google.com> wrote:\n>> On Sun, May 20, 2018 at 3:50 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n>>> It is apparently undefined behavior to call `regfree()` on a regex where\n>>> `regcomp()` failed. [...]\n>>>\n>>> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n>>> @@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)\n>>>                 /* The POSIX.2 people are surely sick */\n>>>                 char errbuf[1024];\n>>>                 regerror(err, regex, errbuf, 1024);\n>>> -               regfree(regex);\n>>>                 die(\"invalid regex: %s\", errbuf);\n>>\n>> While the commit message is very clear why we supposedly introduce a leak here,\n>> it is hard to be found from the source code (as we only delete code\n>> there, so digging\n>> for history is not obvious), so maybe\n>>\n>>      /* regfree(regex) is invalid here */\n>>\n>> instead?\n>\n> The commit message doesn't say that we are supposedly introducing a\n> leak (and, indeed, no resources should have been allocated to the\n> 'regex' upon failed compile). It's saying that removing this call\n> potentially avoids a crash under some implementations.\n>\n> Given that the very next line is die(), and that the function name has\n> \"_or_die\" in it, I'm not sure that an in-code comment about regfree()\n> being invalid upon failed compile would be all that helpful; indeed,\n> it could be confusing, causing the reader to wonder why that is\n> significant if we're just dying anyhow. I find that the patch, as is,\n> clarifies rather than muddles the situation.\n\nLike Eric, I feel that the possible leak here is somewhat uninteresting,\nsince the next line will die. That said, I seem to recall from my\ngrepping around earlier that we have other users where we return\nwith a failure instead of dying.\n\nAny clarifying comments in such code would be a separate patch to me. I\nalso do not immediately see the need for adding such a comment in those\nplaces. We can do that once we verify that we actually do leak (I would\nexpect that to happen only in some implementations, and I think there is\na fair chance that we will never encounter such an implementation.)\n\nMartin\n"},{"id":"348358","messageId":"CAPig+cSR=GnXvOcsxZmskmdQTM+59SpUis28y7RfhfA_-MGi3w@mail.gmail.com","threadId":"48478","inReplyTo":"cover.1526812503.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/3] config: free resources of `struct config_store_data`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-23T07:01:35Z","receivedAt":"2018-05-23T07:01:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 20, 2018 at 6:42 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n> On 14 May 2018 at 05:03, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Sun, May 13, 2018 at 5:58 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n>>> How about the following two patches as patches 2/3 and 3/3? I would also\n>>> need to mention in the commit message of this patch (1/3) that the\n>>> function will soon learn to clean up more members.\n>>\n>> Yep, making this a multi-part patch series and updating the commit\n>> message of the patch which introduces config_store_data_clear(), as\n>> you suggest, makes sense. The patch series could be organized\n>> differently -- such as first moving freeing of 'value_regex' into new\n>> config_store_data_clear(), then freeing additional fields in later\n>> patches -- but I don't think it matters much in practice, so the\n>> current organization is likely good enough.\n>\n> I tried such a re-ordering but wasn't entirely happy about the result\n> (maybe I didn't try hard enough), so here are these patches again, as a\n> proper series and with improved commit messages.\n\nThe re-roll looks good; it address my concern about v1. Thanks.\n"},{"id":"348359","messageId":"CAPig+cSsbHXsVAzhSeah9cPsOha5dUGkxwwAYqku+K3d4p1ttQ@mail.gmail.com","threadId":"48478","inReplyTo":"e71b2abd7298de0123a6222186a8faa3fadf50c0.1526812503.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 3/3] config: let `config_store_data_clear()` handle `key`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-23T07:03:47Z","receivedAt":"2018-05-23T07:03:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 20, 2018 at 6:42 AM, Martin Ågren <martin.agren@gmail.com> wrote:\n> Instead of remembering to free `key` in each code path, let\n> `config_store_data_clear()` handle that.\n>\n> We still need to free it before replacing it, though. Move that freeing\n> closer to the replacing to be safe. Note that in that same part of the\n> code, we can no longer set `key` to the original pointer, but need to\n> `xstrdup()` it.\n\nThat casting away of 'const' was an oddball case anyhow, so it's nice\nto see it go away (even at the expense of an extra xstrdup()).\n\nOverall, this change makes it quite a bit easier to reason about the\ncleanup of 'store.key'.\n\nThanks.\n\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n"}]}