config: free resources of `struct config_store_data`

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.

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.

Signed-off-by Martin Ågren <[email protected]>

---

Quoted 5 lines · Martin Ågren

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.

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.

Instead of carefully clearing up value_regex in each code path, let config_store_data_clear() handle that.

Signed-off-by Martin Ågren <[email protected]>

---
I *think* that it should be ok to regfree() after regcomp() failed, but I'll need to look into that some more (and say something about it in the commit message).

My research (for instance [1,2]) seems to indicate that it would be better to avoid regfree() upon failed regcomp(), even though such a situation is handled sanely in some implementations.

[1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html
[2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html

It is apparently undefined behavior to call regfree() on a regex where regcomp() failed. The language in [1] is a bit muddy, at least to me, but the clearest hint is this (preg is the regex_t *):

Upon successful completion, the regcomp() function shall return 0. Otherwise, it shall return an integer value indicating an error as described in <regex.h>, and the content of preg is undefined.

Funnily enough, there is also the regerror() function which should be given a pointer to such a "failed" regex_t -- the content of which would supposedly be undefined -- and which may investigate it to come up with a detailed error message.

In any case, the example in that document shows how regfree() is not called after regcomp() fails.

We have quite a few users of this API and most get this right. These three users do not.

Several implementations can handle this just fine [2] and these code paths supposedly have not wreaked havoc or we'd have heard about it. (These are all in code paths where git got bad input and is just about to die anyway.) But let's just avoid the issue altogether.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html

[2] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html

Researched-by Eric Sunshine <[email protected]>

Signed-off-byi Martin Ågren <[email protected]>
---

Quoted 6 lines · Eric Sunshine

My research (for instance [1,2]) seems to indicate that it would be better to avoid regfree() upon failed regcomp(), even though such a situation is handled sanely in some implementations.

[1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html
[2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html

Thank you for researching this. I think it would make sense to get rid of the few places we have where we get this wrong (if our understanding of this being undefined is right).

 diffcore-pickaxe.c | 1 -
 grep.c             | 2 --
 2 files changed, 3 deletions(-)

diffcore-pickaxe.c +0 -0

index 239ce5122b..800a899c86 100644

diffcore-pickaxe.c +0 -0

                die("invalid regex: %s", errbuf);
        }
 }

grep.c +0 -3

index 65b90c10a3..5e4f3f9a9d 100644
@@ -636,7 +636,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
        if (err) {
                char errbuf[1024];
                regerror(err, &p->regexp, errbuf, sizeof(errbuf));
-               regfree(&p->regexp);
                compile_regexp_failed(p, errbuf);
        }
 }

@@ -701,7 +700,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
        if (err) {
                char errbuf[1024];
                regerror(err, &p->regexp, errbuf, 1024);
-               regfree(&p->regexp);
                compile_regexp_failed(p, errbuf);
        }
 }
--

2.17.0.840.g5d83f92caf

diffcore-pickaxe.c static void regcomp_or_die(regex_t *regex, const char *needle, int cflags) · line 218

Quoted 10 lines · Martin Ågren

It is apparently undefined behavior to call regfree() on a regex where regcomp() failed. [...]

diffcore-pickaxe.c +0 -1

@@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)
                /* The POSIX.2 people are surely sick */
                char errbuf[1024];
                regerror(err, regex, errbuf, 1024);
-               regfree(regex);
                die("invalid regex: %s", errbuf);

While the commit message is very clear why we supposedly introduce a leak here, it is hard to be found from the source code (as we only delete code there, so digging
for history is not obvious), so maybe

     /* regfree(regex) is invalid here */

instead?

Quoted 17 lines · Stefan Beller

diffcore-pickaxe.c static void regcomp_or_die(regex_t *regex, const char *needle, int cflags) · line 218

Quoted 10 lines · Martin Ågren

It is apparently undefined behavior to call regfree() on a regex where regcomp() failed. [...]

diffcore-pickaxe.c +0 -1

@@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)
                /* The POSIX.2 people are surely sick */
                char errbuf[1024];
                regerror(err, regex, errbuf, 1024);
-               regfree(regex);
                die("invalid regex: %s", errbuf);

While the commit message is very clear why we supposedly introduce a leak here, it is hard to be found from the source code (as we only delete code there, so digging
for history is not obvious), so maybe

Quoted 3 lines

     /* regfree(regex) is invalid here */

instead?

The commit message doesn't say that we are supposedly introducing a leak (and, indeed, no resources should have been allocated to the 'regex' upon failed compile). It's saying that removing this call potentially avoids a crash under some implementations.

Given that the very next line is die(), and that the function name has "_or_die" in it, I'm not sure that an in-code comment about regfree() being invalid upon failed compile would be all that helpful; indeed, it could be confusing, causing the reader to wonder why that is significant if we're just dying anyhow. I find that the patch, as is, clarifies rather than muddles the situation.

Quoted 7 lines · Eric Sunshine

Quoted 5 lines · Martin Ågren

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.

Quoted 7 lines

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.

How about the following two patches as patches 2/3 and 3/3? I would also need to mention in the commit message of this patch (1/3) that the function will soon learn to clean up more members.

I could of course squash the three patches into one, but there is some minor trickery involved, like we can't reuse a pointer in one place, but need to xstrdup it.

Thank you for your comments. I'd be very interested in your thoughts on this.

Quoted 5 lines · Martin Ågren

How about the following two patches as patches 2/3 and 3/3? I would also need to mention in the commit message of this patch (1/3) that the function will soon learn to clean up more members.

Yep, making this a multi-part patch series and updating the commit message of the patch which introduces config_store_data_clear(), as you suggest, makes sense. The patch series could be organized differently -- such as first moving freeing of 'value_regex' into new config_store_data_clear(), then freeing additional fields in later patches -- but I don't think it matters much in practice, so the current organization is likely good enough.

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 <[email protected]>

Quoted 5 lines · Eric Sunshine

Quoted 3 lines · Martin Ågren

How about the following two patches as patches 2/3 and 3/3? I would also need to mention in the commit message of this patch (1/3) that the function will soon learn to clean up more members.

Quoted 7 lines

Yep, making this a multi-part patch series and updating the commit message of the patch which introduces config_store_data_clear(), as you suggest, makes sense. The patch series could be organized differently -- such as first moving freeing of 'value_regex' into new config_store_data_clear(), then freeing additional fields in later patches -- but I don't think it matters much in practice, so the current organization is likely good enough.

I tried such a re-ordering but wasn't entirely happy about the result (maybe I didn't try hard enough), so here are these patches again, as a proper series and with improved commit messages.

+void config_store_data_clear(struct config_store_data *store)