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
René Scharfe <l.s.r@web.de>
Date
Jul 29, 2014, 00:04 UTC
Message-ID
<53D6E52B.1050006@web.de>
In-Reply-To
<xmqqiomh40ui.fsf@gitster.dls.corp.google.com>
Am 28.07.2014 um 23:42 schrieb Junio C Hamano:
Show 30 quoted lines
> 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.

That's the price we pay for using static variables, no? Callers need to consume them as long as they're fresh and multi-threading is not allowed. Before, callers could use wrong buffer contents, after the patch they could still have a pointer to freed memory, which should be more noticeable in tests.

Getting a strbuf_add_real_path() in order to avoid static variables would be nice. And it would also be nice if it worked without calling chdir(). Nice topics for follow-up patches. :)

Show 5 quoted lines
> 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.

It wasn't introduced as an optimization, but to silence valgrind (1d679de5: make_absolute_path: return the input path if it points to our buffer). set_git_work_tree() calls real_path() only once in each of its two branches. However, one caller (init) hands it a path returned by real_path(); we can change that (sent a patch).

René
Previous: Junio C HamanoNext: Junio C Hamano
Message 16 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.