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

Re: [PATCH 1/4] set_git_dir: fix crash when used with real_path()

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 6, 2020, 21:54 UTC
Message-ID
<xmqqa74t2lpr.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<f7afcb4cc83a955b04283475facc02349207557c.1583521396.git.gitgitgadget@gmail.com>

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 18 quoted lines
> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> `real_path()` returns result from a shared buffer, inviting subtle
> reentrance bugs. One of these bugs occur when invoked this way:
>     set_git_dir(real_path(git_dir))
>
> In this case, `real_path()` has reentrance:
>     real_path
>     read_gitfile_gently
>     repo_set_gitdir
>     setup_git_env
>     set_git_dir_1
>     set_git_dir
>
> Later, `set_git_dir()` uses its now-dead parameter:
>     !is_absolute_path(path)
>
> Fix this by using a dedicated `strbuf` to hold `strbuf_realpath()`.

With this detailed explanation, I expected to see a test or two that demonstrates a breakage, but reading a stale value may not reproducibly give the same wrong result or crash the program, perhaps?

Show 16 quoted lines
> -void set_git_dir(const char *path)
> +void set_git_dir(const char *path, int make_realpath)
>  {
> +	struct strbuf realpath = STRBUF_INIT;
> +
> +	if (make_realpath) {
> +		strbuf_realpath(&realpath, path, 1);
> +		path = realpath.buf;
> +	}
> +
>  	set_git_dir_1(path);
>  	if (!is_absolute_path(path))
>  		chdir_notify_register(NULL, update_relative_gitdir, NULL);
> +
> +	strbuf_release(&realpath);
>  }

Makes sense. I looked at changes to the callers in this patch and it all made sense.

Previous: Alexandr Miloslavskiy via GitGitGadgetNext: Alexandr Miloslavskiy
Message 3 of 17 in “Fix bugs related to real_path()”
  1. 0/4 Fix bugs related to real_path()Alexandr Miloslavskiy via GitGitGadget, Mar 6, 2020
  2. 1/4 set_git_dir: fix crash when used with real_path()Alexandr Miloslavskiy via GitGitGadget, Mar 6, 2020
  3. Junio C HamanoMar 6, 2020
  4. Alexandr MiloslavskiyMar 6, 2020
  5. 4/4 get_superproject_working_tree(): return strbufAlexandr Miloslavskiy via GitGitGadget, Mar 6, 2020
  6. Junio C HamanoMar 6, 2020
  7. Alexandr MiloslavskiyMar 6, 2020
  8. 2/4 real_path: remove unsafe APIAlexandr Miloslavskiy via GitGitGadget, Mar 6, 2020
  9. Junio C HamanoMar 6, 2020
  10. Alexandr MiloslavskiyMar 6, 2020
  11. 3/4 real_path_if_valid(): remove unsafe APIAlexandr Miloslavskiy via GitGitGadget, Mar 6, 2020
  12. Junio C HamanoMar 6, 2020
  13. 0/4 Fix bugs related to real_path()Alexandr Miloslavskiy via GitGitGadget, Mar 10, 2020
  14. 4/4 get_superproject_working_tree(): return strbufAlexandr Miloslavskiy via GitGitGadget, Mar 10, 2020
  15. 3/4 real_path_if_valid(): remove unsafe APIAlexandr Miloslavskiy via GitGitGadget, Mar 10, 2020
  16. 2/4 real_path: remove unsafe APIAlexandr Miloslavskiy via GitGitGadget, Mar 10, 2020
  17. 1/4 set_git_dir: fix crash when used with real_path()Alexandr Miloslavskiy via GitGitGadget, Mar 10, 2020

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.