Re: [PATCH v2] path.c enter_repo(): fix unproper strbuf unwrapping and memory leakage

From: Jeff King <peff@peff.net>
Date: Mar 28, 2016, 17:55 UTC
Message-ID: <20160328175528.GB20028@sigill.intra.peff.net>
In-Reply-To: <1459180570-5521-1-git-send-email-huiyiqun@gmail.com>

On Mon, Mar 28, 2016 at 11:56:10PM +0800, Hui Yiqun wrote:

Show 6 quoted lines

> According to strbuf.h, strbuf_detach is the sole supported method
> to unwrap a memory buffer from its strbuf shell.
> 
> So we should not return the pointer of strbuf.buf directly.
> 
> What's more, some memory leakages are solved.

There is something else going on here, which makes this case different than some others. Note that the function returns a const string:

Show 5 quoted lines

> diff --git a/path.c b/path.c
> index 969b494..b07e5a7 100644
> --- a/path.c
> +++ b/path.c
> @@ -625,6 +625,7 @@ const char *enter_repo(const char *path, int strict)

By convention, that means that the result is not owned by the caller, and should not be freed. We implement that by:

>  {
>  	static struct strbuf validated_path = STRBUF_INIT;
>  	static struct strbuf used_path = STRBUF_INIT;

...using static variables which persist after the call returns. So this function retains ownership of the memory, and it remains valid until the next call to enter_repo().

There's no leak when we return NULL, because the function retains control of the memory (though it will hold onto it until the end of the program if nobody calls enter_repo() again). And thus we shouldn't use strbuf_detach(), which loses that reference to the memory (and since the callers don't take ownership, it actually _creates_ a leak).

We could release the memory when returning, but I don't think it's a big deal to do so.

-Peff