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

[惠轶群 <huiyiqun@gmail.com>]

2016-03-29 1:55 GMT+08:00 Jeff King <peff@peff.net>:
> On Mon, Mar 28, 2016 at 11:56:10PM +0800, Hui Yiqun wrote:
>
>> 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:
>
>> 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().

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