Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

From: Jeff King <peff@peff.net>
Date: Oct 25, 2016, 15:15 UTC
Message-ID: <20161025151524.y7wwtetohhqgcvob@sigill.intra.peff.net>
In-Reply-To: <CACsJy8DZWN0RRaDy9w2BVG5pn4FWCY=1YQDsP0V5obrr1wSzZQ@mail.gmail.com>

On Tue, Oct 25, 2016 at 07:38:30PM +0700, Duy Nguyen wrote:

Show 14 quoted lines

> > diff --git a/environment.c b/environment.c
> > index cd5aa57..b1743e6 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -164,8 +164,11 @@ static void setup_git_env(void)
> >         const char *replace_ref_base;
> >
> >         git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > -       if (!git_dir)
> > +       if (!git_dir) {
> > +               if (!startup_info->have_repository)
> > +                       die("BUG: setup_git_env called without repository");
> 
> YES!!! Thank you for finally fixing this.

Good, I'm glad somebody besides me is excited about this. I've been wanting to write this patch for a long time, but it took years of chipping away at all the edge cases.

Show 12 quoted lines

> The "once we've identified" part could be tricky though. This message
> alone will not give us any clue where it's called since it's buried
> deep in git_path() usually, which is buried deep elsewhere. Without
> falling back to core dumps (with debug info), glibc's backtrace
> (platform specifc), the best we could do is turn git_path() into a
> macro that takes __FILE__ and __LINE__ and somehow pass the info down
> here, but "..." in macros is C99 specific, sigh..
> 
> Is it too bad to turn git_path() into a macro when we know the
> compiler is C99 ? Older compilers will have no source location info in
> git_path(), Hopefully they are rare, which means chances of this fault
> popping up are also reduced.

I think you could conditionally make git_path() and all of its counterparts macros, similar to the way the trace code works. It seems like a pretty maintenance-heavy solution, though. I'd prefer conditionally compiling backtrace(); that also doesn't hit 100% of cases, but at least it isn't too invasive.

But I think I still prefer just letting the corefile and the debugger do their job. This error shouldn't happen much, and when it does, it should be easily reproducible. Getting the bug reporter to give either a reproduction recipe, or to run "gdb git" doesn't seem like that big a hurdle.

For fun, here's a patch that uses backtrace(), but it does not actually print the function names unless you compile with "-rdynamic" (and even then it misses static functions). There are better libraries, but of course that's one more thing for the user to deal with when building.

-Peff

---

diff --git a/usage.c b/usage.c
index 17f52c1b5c..4917c6bdfd 100644
--- a/usage.c
+++ b/usage.c
@@ -5,6 +5,9 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#ifdef HAVE_BACKTRACE
+#include <execinfo.h>
+#endif
 
 static FILE *error_handle;
 static int tweaked_error_buffering;
@@ -24,6 +27,32 @@ void vreportf(const char *prefix, const char *err, va_list params)
 	fputc('\n', fh);
 }
 
+#ifdef HAVE_BACKTRACE
+static void maybe_backtrace(void)
+{
+	void *bt[100];
+	char **symbols;
+	int nr;
+
+	if (!git_env_bool("GIT_BACKTRACE_ON_DIE", 0))
+		return;
+
+	nr = backtrace(bt, ARRAY_SIZE(bt));
+	symbols = backtrace_symbols(bt, nr);
+	if (symbols) {
+		FILE *fh = error_handle ? error_handle : stderr;
+		int i;
+
+		fprintf(fh, "die() called from:\n");
+		for (i = 0; i < nr; i++)
+			fprintf(fh, "  %s\n", symbols[i]);
+		free(symbols);
+	}
+}
+#else
+#define maybe_backtrace()
+#endif
+
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
 	vreportf("usage: ", err, params);
@@ -33,6 +62,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 static NORETURN void die_builtin(const char *err, va_list params)
 {
 	vreportf("fatal: ", err, params);
+	maybe_backtrace();
 	exit(128);
 }