From: Jeff King <hidden> Date: 2016-06-15 22:56:51
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:
and doing a clone or fetch. What's actually happening is that we have
two threads running, each of which dies, fooling the recursion check.
This series attempts to fix it. The errors afterwards look like:
fatal: git remote host hung up unexpectedly
fatal: index-pack failed
which is what was printed pre-cd163d4 (usage.c: detect recursion in die
routines and bail out immediately).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:51
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 <redacted>
---
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(-)
From: Jeff King <hidden> Date: 2016-06-15 22:56:51
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 <redacted>
---
run-command.c | 15 ++++++++++++++-
run-command.h | 10 ++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 22:56:51
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 <redacted>
---
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(+)
From: Eric Sunshine <hidden> Date: 2016-06-15 22:56:51
On Mon, Apr 15, 2013 at 7:08 PM, Jeff King [off-list ref] wrote:
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
s/put it the/emitted/ perhaps?
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
Was this supposed to be s/important/unimportant/?
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 <redacted>
From: Jeff King <hidden> Date: 2016-06-15 22:56:51
On Mon, Apr 15, 2013 at 07:45:03PM -0400, Eric Sunshine wrote:
On Mon, Apr 15, 2013 at 7:08 PM, Jeff King [off-list ref] wrote:
quoted
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
s/put it the/emitted/ perhaps?
I meant s/ it//, but I think "sent the message to..." is probably more
clear.
quoted
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
Was this supposed to be s/important/unimportant/?
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 [off-list ref] wrote:
quoted hunk
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 <redacted>
---
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(-)
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.
+ fputs("BUG: recursion detected in die handler\n", stderr);
+ exit(128);
+}
+
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?
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.
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.
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.
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:56:51
Jeff King wrote:
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.
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?
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.
quoted
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.
Right. My assumption was that we are primarily interested in protecting
against the die_routine. Compat functions should never be calling die.
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.
Of course that is assuming nobody violates that rule, which is part of
the point of the check.
quoted
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.
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.
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
From: Jeff King <hidden> Date: 2016-06-15 22:56:51
On Mon, Apr 15, 2013 at 07:34:07PM -0700, Brandon Casey wrote:
quoted
Right. My assumption was that we are primarily interested in protecting
against the die_routine. Compat functions should never be calling die.
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.
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.
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.
Yeah, it is subtle enough that it may be worth protecting against.
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);
}
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
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?
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
From: Johannes Sixt <hidden> Date: 2016-06-15 22:56:51
Am 4/16/2013 4:50, schrieb Jeff King:
On Mon, Apr 15, 2013 at 07:34:07PM -0700, Brandon Casey wrote:
quoted
quoted
Right. My assumption was that we are primarily interested in protecting
against the die_routine. Compat functions should never be calling die.
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.
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.
This rule has been violated LOOOONG ago, and not only in compat/mingw.c
(see xmalloc in compat/qsort.c, for example).
quoted
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.
Yeah, it is subtle enough that it may be worth protecting against.
Too late.
quoted
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);
}
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.
pthread_key is not a problem, but pthread_once is. It's certainly
solvable, but do we really have to?
-- Hannes
From: Jeff King <hidden> Date: 2016-06-15 22:56:51
On Tue, Apr 16, 2013 at 09:18:46AM +0200, Johannes Sixt wrote:
quoted
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.
pthread_key is not a problem, but pthread_once is. It's certainly
solvable, but do we really have to?
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
From: Johannes Sixt <hidden> Date: 2016-06-15 22:56:51
Am 4/16/2013 15:01, schrieb Jeff King:
On Tue, Apr 16, 2013 at 09:18:46AM +0200, Johannes Sixt wrote:
quoted
quoted
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.
pthread_key is not a problem, but pthread_once is. It's certainly
solvable, but do we really have to?
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?
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
From: Jeff King <hidden> Date: 2016-06-15 22:56:52
On Tue, Apr 16, 2013 at 04:13:56PM +0200, Johannes Sixt wrote:
quoted
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?
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.
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
(*) That includes doing nothing, but does not include ripping out the
recursion check, as it protects us from crashes.
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:
Obviously it would help to wrap it in a "clear_die_counter()" function,
but it would still suffer from the problem that there is no
synchronization. In the moment between incrementing and resetting the
dying counter, another thread (including the main program) could check
it. In practice, this does not happen in the current code, because we
do not start many async threads (and we only die in the main thread once
the async thread dies). But it seems unnecessarily flaky and prone to
future problems.
It would also be possible to use mutexes to make it work reliably, but
I'd be very concerned about increasing the complexity of the die code
path. We would never want a hung thread to prevent the main program
from successfully exiting, for example.
So I think the right solution is just a per-thread counter.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:52
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 <redacted>
---
git-compat-util.h | 1 +
usage.c | 20 ++++++++++++++------
2 files changed, 15 insertions(+), 6 deletions(-)
@@ -49,12 +47,19 @@ static void (*warn_routine)(const char *err, va_list params) = warn_builtin;vreportf("warning: ",warn,params);}+staticintdie_is_recursing_builtin(void)+{+staticintdying;+returndying++;+}+/* If we are in a dlopen()ed .so write to a global variable would segfault*(ugh),sokeepthingsstatic.*/staticNORETURN_PTRvoid(*usage_routine)(constchar*err,va_listparams)=usage_builtin;staticNORETURN_PTRvoid(*die_routine)(constchar*err,va_listparams)=die_builtin;staticvoid(*error_routine)(constchar*err,va_listparams)=error_builtin;staticvoid(*warn_routine)(constchar*err,va_listparams)=warn_builtin;+staticint(*die_is_recursing)(void)=die_is_recursing_builtin;voidset_die_routine(NORETURN_PTRvoid(*routine)(constchar*err,va_listparams)){
From: Jeff King <hidden> Date: 2016-06-15 22:56:52
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 <redacted>
---
run-command.c | 11 +++++++++++
1 file changed, 11 insertions(+)
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 <redacted>
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 <redacted>
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
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:56:58
On Tue, Apr 16, 2013 at 9:44 PM, Jeff King [off-list ref] wrote:
On Tue, Apr 16, 2013 at 04:13:56PM +0200, Johannes Sixt wrote:
quoted
quoted
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?
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.
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.