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

2016-03-29 1:55 GMT+08:00 Jeff King [off-list ref]:

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

quoted

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:

quoted

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:

quoted

 {
      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().

Sorry for my carelessness. I didn't notice it.

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