patchrevision: fix memory leaks with `struct cmdline_pathspec`

We don't free the array `prune_data.path` or the individual strings it points to. Do so by introducing and using `free_cmdline_pathspec()`. To be able to safely free the strings, always use `xstrdup()` when assigning them. That does mean we allocate more memory than we used to, but it also means it is clear who owns the strings and that we can stop leaking those that we do allocate.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 revision.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

On Wed, Sep 20, 2017 at 09:47:26PM +0200, Martin Ågren wrote:

Hmm. From this description (and from looking at the patch), it seems like we could just skip the allocation.

The missing piece of the puzzle is that sometimes we call append_prune_data() to append from argv, and sometimes we use read_pathspec_from_stdin().

So we may literally have a mix-and-match of allocated and unallocated entries, and the only sane way to resolve that is by making them all allocated.

So I think this solves the problem, but I couldn't help notice...

Isn't this whole thing just an argv_array, and this is argv_array_pushv? We even NULL-terminate it manually later on!

So rather than increasing the line count by adding free_cmdline_pathspec, I think we could actually _reduce_ it by converting to an argv array, as below. And then adding in your free would be one extra line.

On Wed, Sep 20, 2017 at 04:25:52PM -0400, Jeff King wrote:

Here it is with a commit message, and that final free added.

Sorry for stealing your patch, but I didn't want to suggest "couldn't you replace this with argv_array" without actually seeing if it was possible. At which point the patch was pretty much done.

-- >8 --
Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

We assemble an array of strings in a custom struct, NULL-terminate the result, and then pass it to parse_pathspec().

But then we never free the array or the individual strings (nor can we do the latter, as they are heap-allocated when they come from stdin but not when they come from the passed-in argv).

Let's swap this out for an argv_array. It does the same thing with fewer lines of code, and it's safe to call argv_array_clear() at the end to avoid a memory leak.

Reported-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 revision.c | 39 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 28 deletions(-)

Hi,

Jeff King wrote:

To be devil's advocate for a moment: why don't we use UNLEAK on the paths passed in from stdin?

It's true that there can be an unbounded number of them, but they all coexist in memory anyway. They are not generated dynamically on the fly. Being able to free them doesn't have any obvious advantage over using exit().

Except... is the idea that this allows the strings from stdin to be freed sooner, as soon as they have been parsed into a "struct pathspec"?

That sounds appealing. The only risk would be if "struct pathspec" holds on to a pointer to one of these strings.

Fortunately parse_pathspec() is careful to strdup any strings it borrows (though it is not documented to do so).

In other words, proposed changes:

 1. Could the commit message describe what effect this would have on
    maximum heap usage, if any?  (In qualitative terms is fine, though
    actual numbers would be even better if it's easy to get them.)
    That would make it easier to justify not using UNLEAK.

 2. Can parse_pathspec get a comment in pathspec.h saying that it
    defensively copies anything it needs from args so the caller is
    free to modify or free it?  That way, it should be more obvious
    to people in the future modifying parse_pathspec() that callers
    may rely on that.  (The current API comment describes argv as
    "command line arguments", which I fear would send the opposite
    message to implementors.)

The code looks good.

Thanks, Jonathan

On Wed, Sep 20, 2017 at 03:48:26PM -0700, Jonathan Nieder wrote:

Well, no...the idea is that this is a function which leaks a bunch of memory, and we shouldn't have to think hard about how often its leak can be triggered or how severe it is. We should just fix it.

I agree with your analysis that we're likely only to leak one set of --stdin input per program invocation (modulo some caller re-opening stdin). And I also agree that it's a waste of memory to hold onto leaked heap that cannot be used.

But mostly I am fundamentally against using UNLEAK() in a case like this, because it does not match either of the properties which justified adding UNLEAK() in the first place:

  1. We are about to exit the program, so the "leak" is only caused by
     the memory going out of scope at that exit.

     By contrast, the revision machinery may be called many times in the
     same program.

  2. The memory remains useful until around the time of program exit.

     This most certainly does not, as it would not even be reachable.

So while it may not do _too_ much harm, aside from increasing peak heap unnecessarily, it's not a precedent I'd like to set. And certainly not when we can fix the leak and reduce the code size at the same time.

A lot of the early interfaces in Git had really confusing memory-ownership semantics. I think we've been slowly moving over the years towards interfaces that spend a bit on extra copies to give simple and consistent semantics (in fact, it was one of the reasons I added argv_array in the first place).

So yes, it's good to double check that parse_pathspec() doesn't want to hold on to the pointers. But I also think that should be the default we're striving for in our APIs.

What wording are you looking for? It was a leak, and now it's gone. The size of the leak depends on how much you feed to --stdin. IMHO using UNLEAK is totally inappropriate for this case, and doesn't even seem like an alternative worth rejecting.

I certainly agree that the pathspec interface could use better documentation. Patches welcome? :)

-Peff

Hi,

Jeff King wrote:

[...]

I think I failed at communicating here. That is not what I meant at all.

The context is that Git (especially older parts of it) suffers from a pretty severe lack of API documentation. For newcomers that is especially obvious --- long-time git contributors, on the other hand, may get used to patterns and common interfaces and may have trouble seeing just how bad the lack of clearly communicated API contracts is.

There is a bit of a "broken window" problem, too: authors of one-off patches may reasonably assume from existing code that this is just the way it is and, lacking examples of how to document an API, add more underdocumented API contracts.

The patch I am replying to tightens the contract for parse_pathspec(). I am not asking for comprehensive documentation of that function --- that would be clearly off-topic for the patch. Instead, I am saying that we should document what we are newly requiring of the function in the patch. That way, the documented contract becomes clearer over time as people document what they are relying on. I think of that as generally a good practice.

In other words, it was not at all obvious that "(2) The memory remains useful until around the time of program exit" did not hold. To a casual reader it instead looks like (2) does hold, and there's no documentation short of delving into the implementation of parse_pathspec() to convince a reader otherwise. The documentation that is present leads to the opposite conclusion.

The assertion (1) that this allocation is going to happen multiple times in a program isn't true either. As you noted, we only read stdin once. But that doesn't matter as long as (2) doesn't hold.

TBH saying that I should write the one-sentence doc patch feels like a cop-out. Just like it is not sustainable for those reviewers that are interested in good test coverage to be the only ones who write tests, I think we cannot avoid treating documentation of API contracts as a shared responsibility.

Thanks and hope that clarifies, Jonathan

On Wed, Sep 20, 2017 at 08:49:00PM -0700, Jonathan Nieder wrote:

I disagree with this bit. In my view that was already the contract for parse_pathspec(), and it is simply poorly documented. You can see it being required elsewhere already (e.g., in line_log_init(), and quite probably in other places that feed custom argv-arrays to setup_revisions).

I guess the point I was trying to make is that I _didn't_ have that opposite conclusion. A sane API does not create hidden memory ownership dependencies (though also as I said, it does not hurt to double check if you think it may be wrong, and certainly documenting also does not hurt).

I don't think I asserted that this allocation happens multiple times. I _did_ assert that the revision machinery may be called multiple times, and I'd rather fix the leak than have people figure out the maximum number of bytes we might leak in a given program. In other words, the more important point of what I wrote is that we should not be reaching for UNLEAK() at all in a function like this, even if you might be able to justify it after analysis.

Hopefully the first paragraph I wrote above explains why I don't see this as changing the contract at all (and therefore making the documentation update orthogonal).

I agree that it's a good practice to leave the whole codebase a little prettier than when you started, even if it's not strictly related to the small change you want to make. And I hope my record of patches shows that I follow through on that practice as a general rule.

But I also think it's easy for reviewers to ask for those orthogonal changes, when it would be a similar amount of work for them to communicate their suggestion in the form of a patch.

-Peff

Jeff King wrote:

[...]

I forgot to say: thanks for writing such a pleasant patch.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[...]

Here's such a patch.

-- 8< --
Subject: pathspec doc: parse_pathspec does not maintain references to args

The command line arguments passed to main() are valid for the life of a program, but the same is not true for all other argv-style arrays (e.g. when a caller creates an argv_array). Clarify that parse_pathspec does not rely on the argv passed to it to remain valid.

This makes it easier to tell that callers like "git rev-list --stdin" are safe and ensures that that is more likely to remain true as the implementation of parse_pathspec evolves.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 pathspec.c | 4 ----
 pathspec.h | 7 +++++++
 2 files changed, 7 insertions(+), 4 deletions(-)

On Wed, Sep 20, 2017 at 09:41:12PM -0700, Jonathan Nieder wrote:

You're welcome. :)

Yay!

This looks great to me. I think the whole pathspec API could use better documentation, but this is certainly a good start.

-Peff

Jonathan Nieder <jrnieder@gmail.com> writes:

Obviously the extra text is better than not having any, but I somehow found "Any arguments used" a bit unsatisfactory. magic_mask and flags are probably also copied ;-) but I wonder if *pathspec is also copied? The second sentence that singles out 'prefix' and 'args' helps to remove such a confusion from a clueless reader like me, and makes me wonder if can take advantage of the existence of them in a more direct way.

	It is safe for the caller to modify or free 'prefix' and
	'args' after calling this function, as copies of them are
	stored in the pathspec structure.

or something like that, perhaps. Adding "(which is freed by calling clear_pathspec())" at the end might even better, as that function does not currently have any docstring.

On 20 September 2017 at 22:36, Jeff King <peff@peff.net> wrote:

No need to be sorry. I wasn't aware of the argv_array thing, thanks for seeing the bigger picture. I haven't looked into the details, or your and Jonathan's discussion, but just from a cursory look this looks way better. It reuses existing infrastructure, and then this:

Martin

Jeff King <peff@peff.net> writes:

Makes sense.  Thanks for cleaning up the mess I made.