From: Johannes Sixt <j.sixt@viscovery.net>
To: Jeff King <peff@peff.net>
Cc: Brandon Casey <drafnel@gmail.com>,
"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH 1/3] usage: refactor die-recursion checks
Date: Tue, 16 Apr 2013 09:18:46 +0200 [thread overview]
Message-ID: <516CFB56.1090805@viscovery.net> (raw)
In-Reply-To: <20130416025024.GA20932@sigill.intra.peff.net>
Am 4/16/2013 4:50, schrieb Jeff King:
> On Mon, Apr 15, 2013 at 07:34:07PM -0700, Brandon Casey wrote:
>
>>> 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).
>> 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.
>> 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
next prev parent reply other threads:[~2013-04-16 7:18 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-15 23:06 [PATCH 0/3] avoid bogus "recursion detected in die handler" message Jeff King
2013-04-15 23:08 ` [PATCH 1/3] usage: refactor die-recursion checks Jeff King
2013-04-15 23:45 ` Eric Sunshine
2013-04-15 23:47 ` Jeff King
2013-04-16 0:11 ` Brandon Casey
2013-04-16 0:42 ` Jeff King
2013-04-16 1:41 ` Jonathan Nieder
2013-04-16 2:34 ` Brandon Casey
2013-04-16 2:50 ` Jeff King
2013-04-16 7:18 ` Johannes Sixt [this message]
2013-04-16 13:01 ` Jeff King
2013-04-16 14:13 ` Johannes Sixt
2013-04-16 19:44 ` [PATCH v2 0/2] avoid bogus "recursion detected in die handler" message Jeff King
2013-04-16 19:46 ` [PATCH v2 1/2] usage: allow pluggable die-recursion checks Jeff King
2013-04-16 19:50 ` [PATCH v2 2/2] run-command: use thread-aware die_is_recursing routine Jeff King
2013-04-16 22:09 ` Junio C Hamano
2013-04-17 0:49 ` [PATCH v2 0/2] avoid bogus "recursion detected in die handler" message Jonathan Nieder
2013-04-17 1:37 ` Jeff King
2013-04-23 21:27 ` Erik Faye-Lund
2013-04-15 23:08 ` [PATCH 2/3] run-command: factor out running_main_thread function Jeff King
2013-04-16 1:45 ` Jonathan Nieder
2013-04-16 2:53 ` Jeff King
2013-04-15 23:09 ` [PATCH 3/3] usage: do not check die recursion outside main thread Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=516CFB56.1090805@viscovery.net \
--to=j.sixt@viscovery.net \
--cc=drafnel@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).