patch, 3 partsavoid bogus "recursion detected in die handler" message

If upload-pack fails while generating a pack, the receiving side of a fetch will print out a message like this:

  fatal: git remote host hung up unexpectedly
  fatal: recursion detected in die handler

You can see it by instrumenting upload-pack to fail like this:

When any git code calls die(), we chain to a custom die_routine, which we expect to print a message and exit the program. To avoid infinite loops, we detect a recursive call to die() with a simple counter, and break out of the loop by printing a message and exiting ourselves, without chaining to the die_routine.

But the user does not get to see the message that would have been fed to the die_routine, which makes debugging harder. The user does not know if it was a true infinite loop, or simply a single re-entrant call, since they cannot compare the messages. Furthermore, if we are wrong about detecting the recursion, we have blocked the user from seeing the original message, which is probably the more useful one.

This patch teaches die() to print the original die message to stderr before reporting the recursion. The custom die_routine may or may not have put it the message to stderr, but this is the best we can do (it is what most handlers will do anyway, and it is where our recursion error will go).

While we're at it, let's mark the "recursion detected" message as a "BUG:", since it should never happen in practice. And let's factor out the repeated code in die and die_errno. This loses the information of which function was called to cause the recursion, but it's important; knowing the actual message fed to the function (which we now do) is much more useful, as it can generally pin-point the actual call-site that caused the recursion.

Signed-off-by: Jeff King <peff@peff.net>
---
This helped me debug the current problem. And factoring it out helps
with patch 3. :)

 usage.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

On Mon, Apr 15, 2013 at 7:08 PM, Jeff King <peff@peff.net> wrote:

s/put it the/emitted/ perhaps?

Was this supposed to be s/important/unimportant/?

On Mon, Apr 15, 2013 at 07:45:03PM -0400, Eric Sunshine wrote:

I meant s/ it//, but I think "sent the message to..." is probably more clear.

Urgh, yes, it was originally "not important" but I lost the "not" while trying to clarify the wording.

Thanks.

-Peff

On Mon, Apr 15, 2013 at 4:08 PM, Jeff King <peff@peff.net> wrote:

How do you know it's safe to call vreportf() ?

If the bug is in the vreportf code path, we will recurse infinitely (at least until the stack is used up). An implementation of vsnprintf exists in compat/snprintf.c for example.

It's nice to print out the error message here, but I think doing so defeats the purpose of this "dying" check. Better to get the stack trace from a core dump.

-Brandon

On Mon, Apr 15, 2013 at 05:11:36PM -0700, Brandon Casey wrote:

Because I hand-audited it. But I think the more important question you are getting at is: how do I know that it will remain safe to call vreportf?

Right. My assumption was that we are primarily interested in protecting against the die_routine. Compat functions should never be calling die. Of course that is assuming nobody violates that rule, which is part of the point of the check.

Easier said than done, sometimes, if you are debugging somebody else's problem from a dump of a terminal session. :)

But I can live with dropping this patch; my primary goal is the bug-fix in patch three.

I was also tempted to suggest just dropping the recursion check altogether. While it is neat to detect such things, it's a "should never happen" bug situation, and an infinite loop of printing out the same message is pretty easy to notice. Do you remember what spurred the original check? The message in cd163d4 doesn't say.

-Peff

Jeff King wrote:

On servers it might be useful to avoid accidentally filling up logs quickly.

IIUC the context is in the following two threads:

 http://thread.gmane.org/gmane.comp.version-control.git/182982
 http://thread.gmane.org/gmane.comp.version-control.git/181421/focus=181443

Thanks, Jonathan

On Mon, Apr 15, 2013 at 5:42 PM, Jeff King <peff@peff.net> wrote:

:)

Right.

I think the rule we have been enforcing is less strict than that. We have only said that any compat function in a die handler path should never call die. But maybe that's what you meant.

That's a valid option.

The primary motivation was that Hannes Sixt had to step in and point out yet again that the high-level memory allocators should not be called in anything that could be in a die handler code path. I was on the thread, but I don't remember the topic (ah, Jonathan has stepped in with the answer). I do remember that I was not the only one who had forgotten about that rule though.

We didn't actually have someone report that they encountered infinite recursion, but it seemed easy enough to add a check for it, so...

Unfortunately, I didn't realize that the async functions installed their own die handler which may not exit the process, allowing die to be called multiple times.

To implement this check correctly/completely (i.e. detect recursion in the main thread as well as in any child threads), I think you really do need to use thread-local storage as you mentioned in 3/3 which could look something like:

   static pthread_key_t dying;
   static pthread_once_t dying_once = PTHREAD_ONCE_INIT;

   void setup_die_counter(void)
   {
           pthread_key_create(&dying, NULL);
   }

   check_die_recursion(void)
   {
           pthread_once(&dying_once, setup_die_counter);
           if (pthread(getspecific(dying)) {
                   puts("BUG: recursion...");
                   exit(128);
           }

           pthread_setspecific(dying, &dying);
   }

or maybe the setup could be performed in set_die_routine(), but it does kinda seem like overkill for a "nicety" like this. So maybe checking for recursion in just the main thread as this series does is better than nothing.

-Brandon

On Mon, Apr 15, 2013 at 07:34:07PM -0700, Brandon Casey wrote:

No, I assumed we were following the stronger rule. If you are a compat function for a C library function, then you should never need to die. You should be conforming to the existing interface, which will have some mechanism for passing back an error.

Yeah, it is subtle enough that it may be worth protecting against.

Yeah, that seems sane; my biggest worry was that it would create headaches for Windows folks, who would have to emulate pthread_key. But it seems like we already added support in 9ba604a.

I'll try to re-work the series with thread-local storage, and I'll leave off the extra printing. This _should_ never happen, so if we are going to put in the check, it is probably better to be more thorough than to worry about what the error message looks like.

Thanks for looking it over.

-Peff

Am 4/16/2013 4:50, schrieb Jeff King:

This rule has been violated LOOOONG ago, and not only in compat/mingw.c (see xmalloc in compat/qsort.c, for example).

Too late.

pthread_key is not a problem, but pthread_once is. It's certainly solvable, but do we really have to?

-- Hannes

On Tue, Apr 16, 2013 at 09:18:46AM +0200, Johannes Sixt wrote:

I'm not clear on what you are suggesting. That we protect only the main thread from recursion, or that we drop the check entirely? Or that we implement thread-local storage for this case without using pthread_once?

-Peff

Am 4/16/2013 15:01, schrieb Jeff King:

Anything(*) that does not require pthread_once. A pthread_once implementation on Windows would be tricky and voluminous and and on top of it very likely to be done differently for gcc and MSVC. I don't like to go there if we can avoid it.

(*) That includes doing nothing, but does not include ripping out the recursion check, as it protects us from crashes.

-- Hannes

On Tue, Apr 16, 2013 at 04:13:56PM +0200, Johannes Sixt wrote:

We don't need to use pthread_once, as we can just do the initialization of the thread-local storage along with starting the first thread (where we already do similar initialization). Patch series to follow:

  [1/2]: usage: allow pluggable die-recursion checks
  [2/2]: run-command: use thread-aware die_is_recursing routine

I don't think doing nothing is a good idea. The recursion-detection is triggering erroneously, blocking real error messages and replacing them with the scary red-herring "recursion detected in die handler".

The absolute simplest thing I think we could do is basically:

When any git code calls die or die_errno, we use a counter to detect recursion into the die functions from any of the helper functions. However, such a simple counter is not good enough for threaded programs, which may call die from a sub-thread, killing only the sub-thread (but incrementing the counter for everyone).

Rather than try to deal with threads ourselves here, let's just allow callers to plug in their own recursion-detection function. This is similar to how we handle the die routine (the caller plugs in a die routine which may kill only the sub-thread).

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h |  1 +
 usage.c           | 20 ++++++++++++++------
 2 files changed, 15 insertions(+), 6 deletions(-)

If we die from an async thread, we do not actually exit the program, but just kill the thread. This confuses the static counter in usage.c's default die_is_recursing function; it updates the counter once for the thread death, and then when the main program calls die() itself, it erroneously thinks we are recursing. The end result is that we print "recursion detected in die handler" instead of the real error in such a case (the easiest way to trigger this is having a remote connection hang up while running a sideband demultiplexer).

This patch solves it by using a per-thread counter when the async_die function is installed; we detect recursion in each thread (including the main one), but they do not step on each other's toes.

Other threaded code does not need to worry about this, as they do not install specialized die handlers; they just let a die() from a sub-thread take down the whole program.

Since we are overriding the default recursion-check function, there is an interesting corner case that is not a problem, but bears some explanation. Imagine the main thread calls die(), and then in the die_routine starts an async call. We will switch to using thread-local storage, which starts at 0, for the main thread's counter, even though the original counter was actually at 1. That's OK, though, for two reasons:

  1. It would miss only the first level of recursion, and
     would still find recursive failures inside the async
     helper.

  2. We do not currently and are not likely to start doing
     anything as heavyweight as starting an async routine
     from within a die routine or helper function.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Jeff King <peff@peff.net> writes:

Will queue; thanks.

Jeff King wrote:

Lovely. This doesn't solve the analagous problem for grep, index-pack, pack-objects, preload-index, or bidirectional_transfer_loop, but it doesn't make them worse and even should make them easier to fix later, so

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

On Tue, Apr 16, 2013 at 05:49:41PM -0700, Jonathan Nieder wrote:

The problem does exist in those other programs, but is much less likely to be triggered. There it would be a race between two threads calling die() at the same time, because as soon as one finishes, it takes down the whole program. What made this one so easy to trigger was the installation of die_async.

So it's probably not that big a deal to leave the race in those other places for the time being. Any refactoring of the "dying" flag should accompany an overall strategy for managing the clean death of sub-threads (which would entail a custom die routine, at which point the recursion-check should just fall out naturally from whatever approach they choose).

Thanks for reviewing.

-Peff

On Tue, Apr 16, 2013 at 9:44 PM, Jeff King <peff@peff.net> wrote:

This seems to have been settled as a "we don't need to go there" already, but just in case; here's an implementation of PTHREAD_MUTEX_INITIALIZER using double-checked locks, and it would probably be repurposable to do pthread_once if needed:

https://github.com/kusma/git/commit/c2d7190ed64652c3ac8ea1f2800ff8e7d0b6c01e

Yes, it's not pretty, it's error-prone; the best thing would be not to do it.

When the async subsystem is used, we may spawn off sub-threads (if the platform supports it), and consider the original thread to be the "main" thread of execution. We use this information in a custom die_routine to decide whether to call pthread_exit or a regular full-process exit.

Let's pull this decision out into its own function so that other parts of the system can use it to make decisions.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 15 ++++++++++++++-
 run-command.h | 10 ++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

Jeff King wrote:

Would it make sense to do something like

	return !main_thread_set || pthread_equal(...);

in case someone tries to call this before the first start_async call?

On Mon, Apr 15, 2013 at 06:45:04PM -0700, Jonathan Nieder wrote:

I considered that, but assumed that pthread_equal would always return false against the zeroed out static main_thread. Mostly because we appeared not to be bothering with such a check already. But actually, in the existing code, it is only checked from die_async, which is only put into place when we _do_ start a thread, so we would never hit this code path without main_thread being set.

So yes, a check probably would make sense. However, I think I'm convinced that we should drop this in favor of using thread-local storage for the "dying" counter. So this patch can just go away.

Thanks for reviewing this version, though.

-Peff

When we have started threads with the async subsystem, both the thread and the main program may call die(). The async subsystem sets up a special die routine to call pthread_exit rather than regular exit, but the recursion check in die() happens outside of this routine, and does not know that it's OK for two dies to run.

As a result, we may print "recursion in die handler" when an async thread dies, even though there is no recursion. This can be triggered, for example, by upload-pack errors; the receiving side will die both in the async sideband demultiplexer and in the main program.

This patch teaches the recursion check to use the same running_main_thread() check that the async code bases its exit decision on. That means we detect only recursion within the main program itself, and assume that async handlers correctly die (which they do).

Signed-off-by: Jeff King <peff@peff.net>
---
This is the simplest solution. You could actually have a thread-local
dying counter, which would detect die recursion within an async thread.
But I did not want to get into designing a lowest-common-denominator of
thread-local storage that would work on all of our platforms.

 usage.c | 4 ++++
 1 file changed, 4 insertions(+)