git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 28, 2014, 21:42 UTC
Message-ID
<xmqqiomh40ui.fsf@gitster.dls.corp.google.com>
In-Reply-To
<20140728191649.GE11265@peff.net>
Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> 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.

Previous: Jeff KingNext: René Scharfe
Message 15 of 28 in “getcwd without PATH_MAX”
  1. 0/10 getcwd without PATH_MAXRené Scharfe, Jul 28, 2014
  2. 01/10 strbuf: add strbuf_getcwd()René Scharfe, Jul 28, 2014
  3. 02/10 unix-sockets: use strbuf_getcwd()René Scharfe, Jul 28, 2014
  4. Jeff KingJul 28, 2014
  5. 03/10 setup: convert setup_git_directory_gently_1 et al. to strbufRené Scharfe, Jul 28, 2014
  6. Eric SunshineJul 28, 2014
  7. Torsten BögershausenAug 16, 2014
  8. René ScharfeAug 16, 2014
  9. Junio C HamanoAug 18, 2014
  10. 04/10 abspath: use strbuf_getcwd() to remember original working directoryRené Scharfe, Jul 28, 2014
  11. 05/10 abspath: convert real_path_internal() to strbufRené Scharfe, Jul 28, 2014
  12. Jeff KingJul 28, 2014
  13. René ScharfeJul 28, 2014
  14. Jeff KingJul 28, 2014
  15. Junio C HamanoJul 28, 2014
  16. René ScharfeJul 29, 2014
  17. Junio C HamanoJul 29, 2014
  18. fixup for 05/10: plug leakRené Scharfe, Jul 29, 2014
  19. 06/10 wrapper: add xgetcwd()René Scharfe, Jul 28, 2014
  20. 07/10 use xgetcwd() to get the current directory or dieRené Scharfe, Jul 28, 2014
  21. 08/10 use xgetcwd() to set $GIT_DIRRené Scharfe, Jul 28, 2014
  22. 09/10 abspath: convert absolute_path() to strbufRené Scharfe, Jul 28, 2014
  23. Jeff KingJul 28, 2014
  24. René ScharfeJul 28, 2014
  25. fixup for 09/10: plug leakRené Scharfe, Jul 29, 2014
  26. 10/10 use strbuf_add_absolute_path() to add absolute pathsRené Scharfe, Jul 28, 2014
  27. 04/10 abspath: use strbuf_getcwd() to remember original working directoryRené Scharfe, Jul 28, 2014
  28. Jeff KingJul 28, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.