Re: Crash on MSYS2 with GIT_WORK_TREE

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 8, 2017, 15:34 UTC
Message-ID: <alpine.DEB.2.20.1703081304130.3767@virtualbox>
In-Reply-To: <xmqqa88w4bbp.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Tue, 7 Mar 2017, Junio C Hamano wrote:

Show 5 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > OK, so it appears that we'd better audit all the callsites of
> > real_pathdup() and see if anybody _assumes_ that the return values are
> > not NULL.  They all need fixing.

Indeed.

> I just looked at 4ac9006f ("real_path: have callers use real_pathdup and
> strbuf_realpath", 2016-12-12) and it seems all hunks that replaces
> xstrdup(real_path(...)) with real_pathdup(...) in the commit share the
> same issue.

Right, I tried to convey that information in my email to which you replied.

> The one in canonicalize_ceiling_entry() looks OK, though.

Yes, it immediately tests whether NULL was returned.

Show 11 quoted lines

> ec9629b3 ("submodule absorbing: fix worktree/gitdir pointers
> recursively for non-moves", 2017-01-25) introduces a new use of
> real_pathdup() and the result is immediately used to call
> connect_work_tree_and_git_dir() without checking its NULL-ness, but
> the argument to new_git_dir is something that came from git_path()
> that was successfully passed to safe_create_leading_directories(),
> so this one should be OK.
> 
> 1c16df23 ("Merge branch 'bw/realpath-wo-chdir'", 2017-01-18) turns a
> few xstrdup(real_path(...)) in dir.c without thinking.  I think that
> evil merge probably should be reverted.

Rather than a heavy-handed reversal, I would really prefer to perform a diligent audit of all real_pathdup() callers and adjust them appropriately.

Turns out that the canonicalize_ceiling_entry() caller is *the only one* handling NULL correctly. All other callers need to be changed.

Will send something out in a moment.

Ciao, Johannes