Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[ian4hu]

Hi Phillip,

Thanks for the follow-up.  All three points make sense; let me
confirm what I plan to do so we are on the same page.

On SIGPIPE: you are right that my wording was wrong.  Blocking is
not ignoring, and the write() fails with EPIPE either way.  What
changes is that the signal is not delivered while it is blocked;
and since a SIGPIPE raised by a write is thread-directed, it stays
pending on the writing thread instead of being picked up by the
main thread.  If that thread never unblocks it, it is never
delivered, so a worker that blocks SIGPIPE simply stops dying of
it.  I left SIGPIPE out precisely to keep the "upstream of a pipe
dies of SIGPIPE" behaviour that common-init.c restores.  The set I
plan to block is SIGINT, SIGHUP, SIGTERM and SIGQUIT.  If you
think SIGPIPE belongs in it too, I will add it.

On the blocking pattern: agreed.  I will block once around the
spawn loop and restore before the join loop, rather than around
each pthread_create(), via a small guard pair

    struct signal_mask_guard guard;

    block_sigchain_signals(&guard);
    for (i = 0; i < nr_threads; i++)
        pthread_create(&t[i], NULL, fn, &data[i]);
    restore_sigchain_signals(&guard);

plus a git_pthread_create() wrapper on top for the single-thread
call sites.  The loops I have in mind are preload_index(),
index-pack, pack-objects, grep, name-hash and the read-cache entry
loader; the fsmonitor--daemon and simple-ipc threads I would leave
alone for now, as they have their own lifecycle and signal
handling.

On sigaction: agreed as well.  The handled signal is deferred by
default, so the remaining hole is a handler being interrupted by
one of the other common signals that share the same handler.  I
will set sa_mask to sigfillset() and save/restore the whole
struct sigaction, so pop() does not clobber the caller's
sa_flags/sa_mask.

The one gap I cannot close is a thread created by library code
rather than by us.  libcurl's threaded resolver is the case I am
aware of, depending on how libcurl is built; I do not know of an
in-tree instance, and I will mention it in the cover letter rather
than claim it is covered.

So I will drop this patch and build the series along those lines.
As you suggested, I will wait a few days for Peff and Junio to
comment before spending much time on it; if nobody objects I will
send the replacement as v2.

Thanks,
ian4hu

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[Phillip Wood]

On 29/10/2026 14:15, ian4hu wrote:
 >
> I left SIGPIPE out precisely to keep the "upstream of a pipe
> dies of SIGPIPE" behaviour that common-init.c restores.  The set I
> plan to block is SIGINT, SIGHUP, SIGTERM and SIGQUIT.  If you
> think SIGPIPE belongs in it too, I will add it.

I think we want to block all signals when we create a thread, so that 
all signals are handled by the main (initial) thread.

Everything else sounds good

Thanks

Phillip
> On the blocking pattern: agreed.  I will block once around the
> spawn loop and restore before the join loop, rather than around
> each pthread_create(), via a small guard pair
> 
>      struct signal_mask_guard guard;
> 
>      block_sigchain_signals(&guard);
>      for (i = 0; i < nr_threads; i++)
>          pthread_create(&t[i], NULL, fn, &data[i]);
>      restore_sigchain_signals(&guard);
> 
> plus a git_pthread_create() wrapper on top for the single-thread
> call sites.  The loops I have in mind are preload_index(),
> index-pack, pack-objects, grep, name-hash and the read-cache entry
> loader; the fsmonitor--daemon and simple-ipc threads I would leave
> alone for now, as they have their own lifecycle and signal
> handling.
> 
> On sigaction: agreed as well.  The handled signal is deferred by
> default, so the remaining hole is a handler being interrupted by
> one of the other common signals that share the same handler.  I
> will set sa_mask to sigfillset() and save/restore the whole
> struct sigaction, so pop() does not clobber the caller's
> sa_flags/sa_mask.
> 
> The one gap I cannot close is a thread created by library code
> rather than by us.  libcurl's threaded resolver is the case I am
> aware of, depending on how libcurl is built; I do not know of an
> in-tree instance, and I will mention it in the cover letter rather
> than claim it is covered.
> 
> So I will drop this patch and build the series along those lines.
> As you suggested, I will wait a few days for Peff and Junio to
> comment before spending much time on it; if nobody objects I will
> send the replacement as v2.
> 
> Thanks,
> ian4hu

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[Oswald Buddenhagen]

On Thu, Oct 29, 2026 at 04:03:18PM +0000, Phillip Wood wrote:
>On 29/10/2026 14:15, ian4hu wrote:
>> I left SIGPIPE out precisely to keep the "upstream of a pipe
>> dies of SIGPIPE" behaviour that common-init.c restores.  The set I
>> plan to block is SIGINT, SIGHUP, SIGTERM and SIGQUIT.  If you
>> think SIGPIPE belongs in it too, I will add it.
>
>I think we want to block all signals when we create a thread, so that 
>all signals are handled by the main (initial) thread.
>
that wording again does not address the fact that sigpipe is different 
due to being thread-directed [1]. even if the outcome is identical, it 
needs to be reasoned about separately.

[1] https://stackoverflow.com/questions/21718311/when-multi-thread-program-receive-a-sigpipe-signal-because-send-which-thread-wo

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[ian4hu]

Hi Phillip,

Thanks for the follow-up.  All three points make sense; let me
confirm what I plan to do so we are on the same page.

On SIGPIPE: you are right that my wording was wrong.  Blocking is
not ignoring, and the write() fails with EPIPE either way.  What
changes is that the signal is not delivered while it is blocked;
and since a SIGPIPE raised by a write is thread-directed, it stays
pending on the writing thread instead of being picked up by the
main thread.  If that thread never unblocks it, it is never
delivered, so a worker that blocks SIGPIPE simply stops dying of
it.  I left SIGPIPE out precisely to keep the "upstream of a pipe
dies of SIGPIPE" behaviour that common-init.c restores.  The set I
plan to block is SIGINT, SIGHUP, SIGTERM and SIGQUIT.  If you
think SIGPIPE belongs in it too, I will add it.

On the blocking pattern: agreed.  I will block once around the
spawn loop and restore before the join loop, rather than around
each pthread_create(), via a small guard pair

    struct signal_mask_guard guard;

    block_sigchain_signals(&guard);
    for (i = 0; i < nr_threads; i++)
        pthread_create(&t[i], NULL, fn, &data[i]);
    restore_sigchain_signals(&guard);

plus a git_pthread_create() wrapper on top for the single-thread
call sites.  The loops I have in mind are preload_index(),
index-pack, pack-objects, grep, name-hash and the read-cache entry
loader; the fsmonitor--daemon and simple-ipc threads I would leave
alone for now, as they have their own lifecycle and signal
handling.

On sigaction: agreed as well.  The handled signal is deferred by
default, so the remaining hole is a handler being interrupted by
one of the other common signals that share the same handler.  I
will set sa_mask to sigfillset() and save/restore the whole
struct sigaction, so pop() does not clobber the caller's
sa_flags/sa_mask.

The one gap I cannot close is a thread created by library code
rather than by us.  libcurl's threaded resolver is the case I am
aware of, depending on how libcurl is built; I do not know of an
in-tree instance, and I will mention it in the cover letter rather
than claim it is covered.

So I will drop this patch and build the series along those lines.
As you suggested, I will wait a few days for Peff and Junio to
comment before spending much time on it; if nobody objects I will
send the replacement as v2.

Thanks,
ian4hu

[PATCH] sigchain: recover from concurrent double-pops of the same signal

[Ian Hu via GitGitGadget]

From: ian4hu <hu2008yinxiang@163.com>

sigchain_push()/sigchain_pop() update signals[sig].n and the handler
stack non-atomically.  When two copies of the same signal are delivered
in quick succession to different threads, both instances of the handler
run the usual "cleanup; sigchain_pop(sig); raise(sig)" pattern
concurrently, and both read the same s->n before either one decrements
it.  Each then installs s->old[s->n - 1], so a two-entry chain unwinds
both entries while the dispatcher keeps pointing at a handler that was
already popped: n ends at 0 while the effective disposition is still
old[1].

The surviving dispatcher then runs that popped handler, whose own
sigchain_pop() hits the "n < 1" guard and does nothing, and the raise()
at the end redelivers to the same stuck dispatcher forever.  The process
neither dies nor continues; every signal-handling thread spins at 100%
CPU and only SIGKILL recovers it.

This is reachable in practice: "git clone --depth 1" over ssh holds two
SIGTERM entries in the chain at the index-pack parking point (clone's
remove_junk_on_signal, then tempfile's remove_tempfiles_on_signal from
the first lockfile), and two SIGTERMs a fraction of a millisecond apart
land on two threads and corrupt the chain.

Rather than serializing the chain mutation, teach the "n < 1" guard in
sigchain_pop() to repair the damage.  Track how many pushes were ever
performed for the signal; when a pop finds the chain empty but the
chain bottom is known, reinstall s->old[0], the disposition that was
in place before the chain was built.  The stuck handler's raise() then
lands on that disposition -- typically SIG_DFL -- and the process dies
by it, exactly as a single delivery would have.

The repair is inert on every legitimate path to the guard: if the
signal was never pushed, nothing is installed; if the chain was fully
unwound by regular pops, s->old[0] is already installed and
reinstalling it changes nothing.  The push/pop fast path is untouched.

A fault-injected A/B run of the minimal reproducer, compiled from
identical sources with and without this change (the gap between the
"read n" and "decrement n" steps of pop widened so both racing pops
reliably read the same value; 300 interleaved pairs per arm) froze the
unpatched arm 121 times out of 300.  The patched arm never froze: all
300 runs reinstated the pre-chain disposition and died by it, and
20/20 single-delivery control runs never reached the repair code.

Signed-off-by: ian4hu <hu2008yinxiang@163.com>
---
    sigchain: recover from concurrent double-pops of the same signal
    
    During git clone --depth 1 over ssh (or any transport that parks the
    client with an in-flight packfile), two SIGTERMs delivered a fraction of
    a millisecond apart can corrupt the sigchain unwind: both fetch threads
    end up looping inside remove_junk_on_signal() at ~200% CPU, the process
    survives both signals, and further SIGTERMs have no effect (only SIGKILL
    recovers it).
    
    Mechanism (git 2.56.0):
    
     1. clone pushes two SIGTERM handlers: remove_junk_on_signal
        (builtin/clone.c:1134), then on the first tempfile activation (e.g.
        shallow.lock) remove_tempfiles_on_signal (tempfile.c:117):
        old[0]=SIG_DFL, old[1]=remove_junk_on_signal, n=2.
     2. Two deliveries that land on two threads run the top handler
        concurrently; both sigchain_pop() calls read the same s->n and both
        install s->old[s->n - 1]: the chain unwinds two levels for two
        deliveries, ending at n=0 while the dispatcher still points at
        old[1].
     3. The surviving dispatcher then chains to a handler that was already
        popped, whose own sigchain_pop() hits the n < 1 guard (a silent
        no-op), and whose raise() redelivers to the same stuck dispatcher
        forever.
    
    This patch takes the "repair after the fact" route instead of
    serializing the chain mutation: sigchain_pop() remembers how many pushes
    were ever performed for the signal, and when it finds the chain empty
    while the chain bottom is known, it reinstalls s->old[0] -- the
    disposition that was in place before the chain was built. The stuck
    handler's raise() then lands on that disposition (typically SIG_DFL) and
    the process dies by it, exactly as a single delivery would have.
    
    The repair is inert on every legitimate path to the n < 1 guard: if the
    signal was never pushed nothing is installed, and if the chain was fully
    unwound by regular pops, s->old[0] is already installed so reinstalling
    it changes nothing. The push/pop fast path is untouched -- no locking,
    no signal-mask manipulation, no Windows-specific code.
    
    Validation: a minimal reproducer (two threads parked in read(), the same
    push/pop chain, first handler entry doing real close()+unlink() work),
    compiled from identical sources with and without this change. With a
    fault injector widening the gap between the "read n" and "decrement n"
    steps of pop so both racing pops reliably read the same value (300
    interleaved pairs per arm), the unpatched arm froze 121/300 runs while
    the patched arm never froze: all 300 runs reinstated the pre-chain
    disposition and died by it. 20/20 single-delivery control runs never
    reached the repair code.
    
    An alternative fix that serializes sigchain_push()/sigchain_pop() with a
    per-signal spinlock exists as well; both eliminate the freeze. The lock
    prevents the corruption, this patch heals it; they are independent and
    either one suffices.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2263%2Fian4hu%2Fsigchain-recover-from-concurrent-pops-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2263/ian4hu/sigchain-recover-from-concurrent-pops-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2263

 sigchain.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/sigchain.c b/sigchain.c
index 66123bdbab..f6da68ce4d 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -7,6 +7,7 @@ struct sigchain_signal {
 	sigchain_fun *old;
 	int n;
 	int alloc;
+	int pushed;
 };
 static struct sigchain_signal signals[SIGCHAIN_MAX_SIGNALS];
 
@@ -26,6 +27,7 @@ int sigchain_push(int sig, sigchain_fun f)
 	if (s->old[s->n] == SIG_ERR)
 		return -1;
 	s->n++;
+	s->pushed++;
 	return 0;
 }
 
@@ -33,8 +35,21 @@ int sigchain_pop(int sig)
 {
 	struct sigchain_signal *s = signals + sig;
 	check_signum(sig);
-	if (s->n < 1)
+	if (s->n < 1) {
+		/*
+		 * We are unwinding a chain that is already empty.  If we
+		 * got here from a signal handler, two concurrent pops of
+		 * the same signal both decremented "n", leaving the
+		 * dispatcher stuck on a handler that no longer has a
+		 * chain entry: its raise() would re-enter it forever.
+		 * Reinstall the disposition that was in place before the
+		 * chain was built so the raise() that follows lands on
+		 * that instead.
+		 */
+		if (s->pushed)
+			signal(sig, s->old[0]);
 		return 0;
+	}
 
 	if (signal(sig, s->old[s->n - 1]) == SIG_ERR)
 		return -1;

base-commit: f57c9a73884400380bddd172acc88b569834f909
-- 
gitgitgadget

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[ian4hu]

Hi,

Gentle ping on this one.  It is queued in 'seen' as
iy/sigchain-recover-concurrent-pops, and the latest "What's cooking"
marks it as "Needs review".

A short recap: when two copies of the same signal reach two threads
nearly at the same time, the concurrent sigchain_pop() calls in the
handlers can unwind the chain by two levels instead of one, leaving
the process spinning in a handler that keeps re-raising the signal.
This is reachable with "git clone --depth 1" over ssh receiving two
SIGTERMs in quick succession, and only SIGKILL gets rid of the
process.

The patch leaves the push/pop fast path alone and only teaches the
"n < 1" guard in sigchain_pop() to fall back to the pre-chain
disposition, so the stuck handler's raise() terminates the process
just as a single delivery would.

Peff, I have Cc'ed you since you wrote the sigchain code.  I would
appreciate your thoughts on whether this "repair after the fact"
approach is acceptable, or whether you would rather see the chain
mutation serialized instead.  I also have a version that uses a
per-signal spinlock around push/pop, and can send it if that is
preferred.

Thanks,
ian4hu

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[Phillip Wood]

On 28/10/2026 13:37, ian4hu wrote:
> Hi,
> 
> Gentle ping on this one.  It is queued in 'seen' as
> iy/sigchain-recover-concurrent-pops, and the latest "What's cooking"
> marks it as "Needs review".
> 
> A short recap: when two copies of the same signal reach two threads
> nearly at the same time, the concurrent sigchain_pop() calls in the
> handlers can unwind the chain by two levels instead of one, leaving
> the process spinning in a handler that keeps re-raising the signal.

It sounds to me like the bug is that we're unwinding the same chain of 
signal handlers in two different threads at the same time. Doesn't that 
mean we're potentially calling the same handler twice, once from each 
thread, which doesn't sound like it'll lead to anything good. I wonder 
how hard it would be to fix the code to ensure that signals are blocked 
when we spawn a new thread so that they are only handled in a single 
thread. It would probably be good idea to start using sigaction() as 
well so that we don't interrupt a signal handler if we receive another 
instance of the same signal while the handler is running.

Thanks

Phillip

> This is reachable with "git clone --depth 1" over ssh receiving two
> SIGTERMs in quick succession, and only SIGKILL gets rid of the
> process.
> 
> The patch leaves the push/pop fast path alone and only teaches the
> "n < 1" guard in sigchain_pop() to fall back to the pre-chain
> disposition, so the stuck handler's raise() terminates the process
> just as a single delivery would.
> 
> Peff, I have Cc'ed you since you wrote the sigchain code.  I would
> appreciate your thoughts on whether this "repair after the fact"
> approach is acceptable, or whether you would rather see the chain
> mutation serialized instead.  I also have a version that uses a
> per-signal spinlock around push/pop, and can send it if that is
> preferred.
> 
> Thanks,
> ian4hu

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[ian4hu]

Hi Phillip,

Thanks for the review.  I agree with both halves of your mail, and
I think the difference between the two approaches is worth spelling
out.

What I sent only repairs the aftermath.  It leaves the corrupted
state reachable and merely turns the wedge into the normal death, so
it is small and inert on the legitimate paths, but it is a band-aid
and, frankly, a bit dirty: it heals a state that should never have
been entered, and by the time it heals it the two threads have
already run the handler's close()/unlink() cleanup concurrently.

I did first try the serializing route.  My earlier attempt
(https://github.com/gitgitgadget/git/pull/2262, never sent to the
list) took a per-signal spinlock around the push/pop update, with
the signal blocked while the lock is held.  That keeps
signals[sig].n and the installed disposition consistent under any
interleaving, so the corrupt state cannot be constructed at all,
and it also protects sigchain_push/pop called from worker threads.
I dropped it because of the edges:

  - two threads can still run the handler body concurrently; the
    lock only makes the unwind atomic, it does not serialize the
    cleanup itself;
  - the POSIX corner is awkward: sigprocmask() is unspecified in a
    multithreaded process, pthread_sigmask() is not
    async-signal-safe, and taking the lock in a handler needs one
    of them;
  - a thread that dies inside the critical section leaves the lock
    held and the next delivery spins forever, trading a hang for a
    hang;
  - the Windows branch needs its own InterlockedExchange path that
    cannot be exercised in CI here.

So the question I would like your guidance on: do you consider a
locked/serialized chain a complete fix for what you described, or
does this have to be prevented at the thread level - block the
signals when a thread is spawned, and use sigaction() for the
chain?  My own reading is that serializing is a complete fix for
the chain state but not for the handler bodies, so if the latter is
what you want, your suggestion is the way to go and the lock does
not buy much beyond it.

If you prefer the thread-level fix, I am happy to build it: a
git_pthread_create() wrapper that blocks SIGINT/SIGHUP/SIGTERM/
SIGQUIT around pthread_create() - leaving SIGPIPE alone, since it is
a synchronous signal and blocking it in workers turns deaths into
EPIPE - plus a sigaction()-based chain that masks all signals while
a handler runs.  I would then drop this patch, or keep it as a
separate last-resort series if you think the belt and braces are
worth it.

Thanks,
ian4hu

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[Phillip Wood]

On 28/10/2026 14:47, ian4hu wrote:
> 
> If you prefer the thread-level fix, I am happy to build it: a
> git_pthread_create() wrapper that blocks SIGINT/SIGHUP/SIGTERM/
> SIGQUIT around pthread_create()

I think that is probably the safest solution (though it does not help 
for threads created external library code - I'm not sure if we have any 
instances of that). I'd be interested to hear what others think, so I'd 
wait a couple of days to see if anyone else comments before spending 
much time on it.

> - leaving SIGPIPE alone, since it is
> a synchronous signal and blocking it in workers turns deaths into
> EPIPE

Does it? I thought that happened when its disposition is set to SIG_IGN. 
Blocking a signal delays its delivery until it is unblocked, it does not 
ignore the signal.

> - plus a sigaction()-based chain that masks all signals while
> a handler runs.

When I wrote my last email I was thinking about blocking the signal that 
is being handled as sigaction() defers the signal that is being handled 
unless SA_NODEFER is passed. However, sigchain_push_common() sets the 
same handler for all the common signals so it is probably safer to block 
at least those as well as I'm not sure all our handlers are reentrant.

>  I would then drop this patch, or keep it as a
> separate last-resort series if you think the belt and braces are
> worth it.

I'd be inclined to drop it - if we're handling signals in a single 
thread that should avoid the issue.

Thanks

Phillip

Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

[Phillip Wood]

A couple more thoughts ...

On 28/10/2026 16:39, Phillip Wood wrote:
> On 28/10/2026 14:47, ian4hu wrote:
>>
>> If you prefer the thread-level fix, I am happy to build it: a
>> git_pthread_create() wrapper that blocks SIGINT/SIGHUP/SIGTERM/
>> SIGQUIT around pthread_create()
> 
> I think that is probably the safest solution (though it does not help 
> for threads created external library code - I'm not sure if we have any 
> instances of that). I'd be interested to hear what others think, so I'd 
> wait a couple of days to see if anyone else comments before spending 
> much time on it.

We have a number of places where we spin up a bunch of threads to 
parallelize some work, for example in preload_index() we create some 
threads in a loop to read the index. In cases like that, where the loop 
body is just setting up some per-thread data and creating a thread, it 
would be more efficient to block signals before the loop and unblock 
them after, rather than blocking and unblocking each time we create a 
thread.

>> - leaving SIGPIPE alone, since it is
>> a synchronous signal and blocking it in workers turns deaths into
>> EPIPE
> 
> Does it? I thought that happened when its disposition is set to SIG_IGN. 
> Blocking a signal delays its delivery until it is unblocked, it does not 
> ignore the signal.

To clarify, in a multi threaded process the delivery of the signal is 
only delayed if it is blocked in all threads. If there's one or more 
threads where the signal isn't blocked it will be delivered to one of 
those threads.

Thanks

Phillip

>> - plus a sigaction()-based chain that masks all signals while
>> a handler runs.
> 
> When I wrote my last email I was thinking about blocking the signal that 
> is being handled as sigaction() defers the signal that is being handled 
> unless SA_NODEFER is passed. However, sigchain_push_common() sets the 
> same handler for all the common signals so it is probably safer to block 
> at least those as well as I'm not sure all our handlers are reentrant.
> 
>>  I would then drop this patch, or keep it as a
>> separate last-resort series if you think the belt and braces are
>> worth it.
> 
> I'd be inclined to drop it - if we're handling signals in a single 
> thread that should avoid the issue.
> 
> Thanks
> 
> Phillip