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.