Re: [PATCH v3 05/10] abspath: convert real_path_internal() to strbuf

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 28, 2014 at 08:28:30PM +0200, René Scharfe wrote:
>
>> @@ -60,26 +58,22 @@ static const char *real_path_internal(const char *path, int die_on_error)
>>  			goto error_out;
>>  	}
>>  
>> -	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX) {
>> -		if (die_on_error)
>> -			die("Too long path: %.*s", 60, path);
>> -		else
>> -			goto error_out;
>> -	}
>> +	strbuf_init(&sb, 0);
>> +	strbuf_addstr(&sb, path);
>
> As with the other patch I just mentioned, should this be strbuf_reset,
> not strbuf_init? We want to reset the static buffer back to zero-size,
> not throw it away and leak whatever was there.
>
> -Peff

Yes, this one seems to be leaking.

"Next call to the function invalidates the return value the last
caller received" feels like playing with fire.  Most existing
callers are safe in that the first thing they do to the returned
string is xstrdup() it, but we would need to check all the other
callers.

I briefly thought it is not OK for set_git_work_tree(), which gets
new_work_tree, calls real_path() to receive the value from the
function, and then calls real_path() again on it.  The "We've
already done it" optimization is the only thing that makes it safe,
which feels overly fragile.