runtime: signal handling: async signals should forward to existing C handlers by default #19465

bcmills · 2017-03-08T22:46:26Z

runtime.sigfwdgo currently includes this snippet, intended to ignore signals per the documented behavior of os/signal:

	// If we aren't handling the signal, forward it.
	// […]
	if atomic.Load(&handlingSig[sig]) == 0 {
		sigfwd(fwdFn, sig, info, ctx)
		return true
	}

	flags := sigtable[sig].flags

	// Only forward synchronous signals and SIGPIPE.
	// […]
	if (c.sigcode() == _SI_USER || flags&_SigPanic == 0) && sig != _SIGPIPE {
		return false
	}
	// Determine if the signal occurred inside Go code. We test that:
	//   (1) we were in a goroutine (i.e., m.curg != nil), and
	//   (2) we weren't in CGO.
	g := getg()
	if g != nil && g.m != nil && g.m.curg != nil && !g.m.incgo {
		return false
	}

handlingSig[sig] is set for all of the signals that the os/signal package cares about any time signal.Notify is called for any signal.

That results in signals intended for C handlers (such as SIGABRT) being dropped, which is arguably incorrect for programs which may include C handlers for those signals.

For example, in the program below we register an early C handler for SIGUSR1 and a Go handler for SIGUSR2, then signal the program with SIGUSR1 (in a way that happens to ensure the signal is delivered to a thread in a Go stack frame, to make the program more deterministic).

Since there is a C handler for the signal and no corresponding signal.Notify call for that signal, sigfwdgo arguably ought to forward SIGUSR1 to the C handler.

Instead, it returns without forwarding, the handler drops through to runtime.sighandler, and the signal is ignored.

bcmills:~/go$ go version
go version devel +228438e097 Wed Mar 8 21:34:32 2017 +0000 linux/amd64

src/asyncsig/asyncsig.go:

package main

/*
#include <signal.h>
#include <stdbool.h>
#include <stdlib.h>

static volatile sig_atomic_t signaled = 0;

static void record_signal(int sig) {
	signaled = 1;
}

static void register_handler() __attribute__ ((constructor (200)));

static void register_handler() {
	signal(SIGUSR1, record_signal);
}

static bool was_signaled() {
	return signaled != 0;
}
*/
import "C"

import (
	"os"
	"os/signal"
	"runtime"
	"syscall"
)

func main() {
	ch := make(chan os.Signal)
	signal.Notify(ch, syscall.SIGUSR2)

	tidc := make(chan int, 1)
	go func() {
		runtime.LockOSThread()
		tid := syscall.Gettid()
		for { // Loop forever sending the current TID.
			select {
			case tidc <- tid:
			default:
			}
		}
	}()

	syscall.Tgkill(syscall.Getpid(), <-tidc, syscall.SIGUSR1)

	// Wait for the thread to finish handling the signal.
	<-tidc
	<-tidc

	if !C.was_signaled() {
		panic("expected SIGUSR1 to have been signaled")
	}
}

bcmills:~$ go build asyncsig && ./asyncsig
panic: expected SIGUSR1 to have been signaled

goroutine 1 [running]:
main.main()
        /usr/local/google/home/bcmills/src/asyncsig/asyncsig.go:56 +0x1a3

The text was updated successfully, but these errors were encountered:

bcmills · 2017-03-08T22:46:39Z

(@ianlancetaylor)

ianlancetaylor · 2017-03-08T23:18:04Z

You mention signal.Notify, but I think that everything you say is true even if signal.Notify is never called. At least, I don't see why it matters whether signal.Notify is called.

When I wrote the os/signal docs, the intent was that in a Go program (as opposed to Go code built with -buildmode=c-archive or similar), the Go signal handler handles signals. In a Go program, we only forward synchronous signals to C code. Non-synchronous signals are always handled within Go. I hope this is all as documented at https://golang.org/pkg/os/signal.

I think you are suggesting that we should extend the signal handling for a Go program as follows: if a C signal handler for SIG is installed before the Go runtime is initialized, and if signal.Notify was not called for SIG, then if the Go signal handler sees SIG it should forward it to the previously installed C signal handler. We already do this for synchronous signals, so we are only talking about non-synchronous signals here. Some non-synchronous signals are handled specially by the Go runtime; there is special handling for at least SIGHUP, SIGINT, SIGTERM, SIGQUIT, SIGILL, SIGTRAP, SIGABRT, SIGSTKFLT, SIGEMT, SIGSYS. Are you thinking that we should change the behavior for those signals if there is a previously installed C signal handler? Or are you only thinking that we should change the behavior for the other signals, those which would be otherwise ignored?

This isn't a case where I see an obvious right choice.

bcmills · 2017-03-09T00:13:40Z

I don't see why it matters whether signal.Notify is called.

You're correct: if I take out the signal.Notify call from my test program, it still fails.

I think you are suggesting […]

Yep, your description looks like what I had in mind.

Some non-synchronous signals are handled specially by the Go runtime […]. Are you thinking that we should change the behavior for those signals if there is a previously installed C signal handler?

It's not obvious to me either.

I would argue that any "reasonable" C handler for SIGQUIT or SIGABRT will end by terminating the program (either by calling exit directly or reinstalling SIG_DFL or some prior handler and re-raising the signal). A SIGABRT handler in particular might do some logging or other input-of-death mitigation, so it seems important to forward the signal in that case. Perhaps for those signals it would make the most sense to go ahead and dump the relevant Go stack (or stacks) and then forward the signal. (I think #19389 might achieve that behavior for SIGABRT, but I'm not so sure about SIGQUIT.)

I would be surprised to see interesting early-constructor handlers for SIGHUP, SIGINT, or SIGTERM since the most useful handlers for those signals are to provide application-specific terminal interaction. I would probably bias toward forwarding: as far as I'm aware the default Go behavior is very similar to the default POSIX behavior, so if there is a non-default handler registered early, it either does approximately what Go was intending to do anyway, or intentionally deviates from that behavior.

I'm not at all sure what we should do for asynchronous delivery of signals that are normally synchronous (SIGILL, SIGTRAP, SIGSTKFLT, SIGEMT, SIGSYS), since I have no idea why someone would send those signals asynchronously in the first place. Perhaps we should do the same as for SIGQUIT or SIGABRT: write out the Go stack dump and then forward (or re-raise) the signal. That way we would still be in keeping with the documented Go behavior (writing a stack dump), but if the C handler does any interesting cleanup (e.g. flushing logs) that will still occur.

bcmills changed the title ~~os/signal: Notify blocks unrelated signals from forwarding to C handlers~~ runtime: signal handling: async signals should forward to existing C handlers by default Mar 9, 2017

bradfitz added this to the Go1.9Maybe milestone Mar 21, 2017

bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017

bcmills mentioned this issue Oct 30, 2017

os/signal: add func Ignored(sig Signal) bool #22497

Closed

rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

This comment has been minimized.

Sign in to view

bcmills mentioned this issue Jan 7, 2020

runtime: signal handling changed by signal.Notify #36420

Closed

tiborvass mentioned this issue Apr 10, 2021

Segfault when calling Go function from cgo signal handler #45499

Closed

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022

bcmills mentioned this issue Feb 15, 2023

runtime: Go crashes in windows when creating JVM using "JNI_CreateJavaVM" #58542

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime: signal handling: async signals should forward to existing C handlers by default #19465

runtime: signal handling: async signals should forward to existing C handlers by default #19465

bcmills commented Mar 8, 2017

bcmills commented Mar 8, 2017

ianlancetaylor commented Mar 8, 2017

bcmills commented Mar 9, 2017 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

runtime: signal handling: async signals should forward to existing C handlers by default #19465

runtime: signal handling: async signals should forward to existing C handlers by default #19465

Comments

bcmills commented Mar 8, 2017

bcmills commented Mar 8, 2017

ianlancetaylor commented Mar 8, 2017

bcmills commented Mar 9, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bcmills commented Mar 9, 2017 •

edited