Re: Crash on MSYS2 with GIT_WORK_TREE

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 8, 2017, 11:59 UTC
Message-ID: <alpine.DEB.2.20.1703081254480.3767@virtualbox>
In-Reply-To: <20170308020918.GA1650@google.com>

Hi Brandon,

On Tue, 7 Mar 2017, Brandon Williams wrote:

Show 33 quoted lines

> On 03/08, Johannes Schindelin wrote:
> > 
> > [...] On *Linux*, this happens:
> > 
> > 	$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
> > 	Segmentation fault (core dumped)
> > 
> > The reason is this: when set_git_work_tree() was converted from using
> > xstrdup(real_path()) to real_pathdup(), we completely missed the fact
> > that the former passed die_on_error = 1 to strbuf_realpath(), while
> > the latter passed die_on_error = 0. As a consequence, work_tree can be
> > NULL now, and the current code does not expect set_git_work_tree() to
> > return successfully after setting work_tree to NULL.
> > 
> > I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers
> > use real_pathdup and strbuf_realpath, 2016-12-12).
> > 
> > Brandon, I have a hunch that pretty much all of the
> > xstrdup(real_path()) -> real_pathdup() sites have a problem now. The
> > previous contract was that real_path() would die() if the passed path
> > is invalid. The new contract is that real_pathdup() returns NULL in
> > such a case. I believe that the following call sites are problematic
> > in particular:
> 
> Welp, looks like I missed that when I made the conversion.  You're
> right, the semantics of getting the real_path were changed which would
> cause a NULL to be returned instead of the program exiting with a call
> to die().  
> 
> After a cursory look at your patch, I think all of your changes look
> sane.  I would have to take a closer look at the call sites to see if
> each caller would need to die or not.  I'm assuming you took a quick
> glace to make your decision about each call site?

I did take a quick glance, but did you have a look at the time of day I sent this patch? You do not want to trust my judgement after that.

Another thing: may I ask you to delete the quoted parts of the mail that you are actually not responding to? Junio also often simply keeps the rest of the mail quoted, and I always have to scroll all the way to the end just to verify that nothing more has been said, which can be slightly annoying when you are tired. I do plan to read your mails in the future, so culling the quoted-yet-unanswered part would save me trouble.

Thanks, Dscho