From 07f66603742fab1a5d51ab60c6efda0ceabc26d8 Mon Sep 17 00:00:00 2001 From: Benjamin Knofe-Vider Date: Fri, 3 Jul 2026 12:27:41 +0200 Subject: [PATCH 1/2] Add native crash handler: die loudly on C-thread fatal signals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A SIGSEGV on a thread the Go runtime does not own (e.g. a DuckDB TaskScheduler thread crashing inside an extension) is routed through the runtime's badsignal path and can leave the process alive but wedged: the crashed thread never releases its locks, and every thread that later touches them blocks forever. In production this turned a postgres_scanner segfault during a JDBC catalog scan into a query that ran 30+ hours, pinning a 15-CPU worker pod and blocking control-plane drains, with no log line anywhere. internal/crashhandler installs (via C constructor, before the Go runtime registers its handlers, so runtime.sigfwdgo chains to it) a native handler for SIGSEGV/SIGBUS/SIGILL/SIGFPE/SIGABRT that writes a grep-able marker + native backtrace to stderr and re-raises with the default disposition, so the process dies and the control plane's existing crash-reap machinery takes over. Faults raised by Go code are not forwarded by the runtime and keep producing ordinary Go panics. Package tests re-exec the test binary and assert death-by-signal plus the stderr marker for a C-thread segfault and a cgo-call segfault, and that a Go nil dereference still panics normally. Not assertable in the e2e harness (no SQL deterministically segfaults a worker) — noted in tests/e2e-mw-dev/README.md. --- cmd/duckgres-controlplane/main.go | 8 ++ cmd/duckgres-worker/main.go | 9 +- internal/crashhandler/crashhandler.go | 30 ++++++ internal/crashhandler/crashhandler_test.go | 114 +++++++++++++++++++++ internal/crashhandler/handler.c | 78 ++++++++++++++ internal/crashhandler/handler_cgo.go | 12 +++ internal/crashhandler/handler_stub.go | 5 + internal/crashhandler/triggers.go | 42 ++++++++ main.go | 10 ++ tests/e2e-mw-dev/README.md | 9 ++ 10 files changed, 316 insertions(+), 1 deletion(-) create mode 100644 internal/crashhandler/crashhandler.go create mode 100644 internal/crashhandler/crashhandler_test.go create mode 100644 internal/crashhandler/handler.c create mode 100644 internal/crashhandler/handler_cgo.go create mode 100644 internal/crashhandler/handler_stub.go create mode 100644 internal/crashhandler/triggers.go diff --git a/cmd/duckgres-controlplane/main.go b/cmd/duckgres-controlplane/main.go index 3d2ba827..1179cb41 100644 --- a/cmd/duckgres-controlplane/main.go +++ b/cmd/duckgres-controlplane/main.go @@ -26,6 +26,7 @@ import ( "github.com/posthog/duckgres/configresolve" "github.com/posthog/duckgres/controlplane" "github.com/posthog/duckgres/internal/cliboot" + "github.com/posthog/duckgres/internal/crashhandler" "github.com/posthog/duckgres/server" ) @@ -51,6 +52,13 @@ func main() { // all-in-one binary. signal.Ignore(syscall.SIGPIPE) + // The native crash handler self-installs from a C constructor (importing + // the package links it in): a fatal signal on a native (cgo) thread must + // kill the process with a native backtrace, not wedge it forever. + if !crashhandler.Installed() { + slog.Warn("Native crash handler NOT installed; fatal signals on native threads may wedge the process.") + } + // CLIInputs-backed flags are registered via the shared helper so this // binary and the all-in-one duckgres binary cannot drift on the // resolver's CLI surface. The bespoke flags below (--config, diff --git a/cmd/duckgres-worker/main.go b/cmd/duckgres-worker/main.go index 28f713a7..9c71e7d5 100644 --- a/cmd/duckgres-worker/main.go +++ b/cmd/duckgres-worker/main.go @@ -21,6 +21,7 @@ import ( "github.com/posthog/duckgres/configresolve" "github.com/posthog/duckgres/duckdbservice" "github.com/posthog/duckgres/internal/cliboot" + "github.com/posthog/duckgres/internal/crashhandler" "github.com/posthog/duckgres/server" ) @@ -45,6 +46,13 @@ func main() { // mid-query. Same rationale as the all-in-one binary. signal.Ignore(syscall.SIGPIPE) + // The native crash handler self-installs from a C constructor (importing + // the package links it in): a SIGSEGV on a DuckDB-created thread must kill + // the worker with a native backtrace, not wedge it forever holding locks. + if !crashhandler.Installed() { + slog.Warn("Native crash handler NOT installed; fatal signals on native threads may wedge the process.") + } + // Worker-relevant flag subset. The all-in-one duckgres binary registers // ~60 flags covering standalone / control-plane / duckdb-service. The // worker only needs the bits that feed server.Config (DuckDB execution, @@ -253,4 +261,3 @@ func main() { MaxSessions: maxSessions, }) } - diff --git a/internal/crashhandler/crashhandler.go b/internal/crashhandler/crashhandler.go new file mode 100644 index 00000000..33fdd614 --- /dev/null +++ b/internal/crashhandler/crashhandler.go @@ -0,0 +1,30 @@ +// Package crashhandler makes native (C/C++) crashes loud instead of silent. +// +// Motivation: duckgres links DuckDB via cgo. A SIGSEGV on a thread the Go +// runtime does not own (e.g. a DuckDB TaskScheduler thread crashing inside an +// extension) is routed through the Go runtime's badsignal path, which can +// leave the process alive but wedged: the crashed thread never releases its +// locks, and every thread that later touches them blocks forever. In +// production this turned a postgres_scanner segfault into a query that ran +// for 30+ hours pinning a worker pod, with no log line anywhere. +// +// Importing this package installs (via a C constructor, before the Go runtime +// initializes its own signal handlers) a native handler for fatal signals that +// +// 1. writes a recognizable marker plus a native backtrace to stderr +// (async-signal-safe: write(2)/backtrace_symbols_fd only), and +// 2. restores the default disposition and re-raises, so the process dies +// with the original signal instead of wedging. +// +// Because the handler is registered before the Go runtime's, the runtime +// saves it as the "previous" handler and forwards non-Go faults to it +// (runtime.sigfwdgo). Faults raised by Go code keep producing ordinary Go +// panics — the handler never sees them. +package crashhandler + +// Marker is the first bytes of the crash report written to stderr. Log +// pipelines can alert on it verbatim. +const Marker = "duckgres: fatal native signal" + +// Installed reports whether the native crash handler is active. +func Installed() bool { return installed() } diff --git a/internal/crashhandler/crashhandler_test.go b/internal/crashhandler/crashhandler_test.go new file mode 100644 index 00000000..0c717536 --- /dev/null +++ b/internal/crashhandler/crashhandler_test.go @@ -0,0 +1,114 @@ +package crashhandler + +import ( + "os" + "os/exec" + "strings" + "syscall" + "testing" +) + +// The tests re-execute the test binary with CRASHHANDLER_TEST_MODE set; TestMain +// dispatches to a crash trigger that must never return. The parent asserts on +// the child's exit status and stderr. +func TestMain(m *testing.M) { + switch os.Getenv("CRASHHANDLER_TEST_MODE") { + case "": + os.Exit(m.Run()) + case "c-thread-segv": + // SIGSEGV on a C-created thread (the incident class: a DuckDB + // TaskScheduler thread faulting inside an extension). + TriggerCThreadSegfault() + case "cgo-call-segv": + // SIGSEGV inside a C call made from a Go goroutine (a DuckDB call + // crashing under cgo). + TriggerCgoCallSegfault() + case "go-nil-deref": + // A plain Go nil dereference must still produce a normal Go panic, + // not a native crash report. + var p *int + _ = *p //nolint:govet + } + // Every mode above must have killed the process. + os.Exit(97) +} + +// runCrashChild re-runs the test binary in the given crash mode and returns +// (stderr, waitStatus). +func runCrashChild(t *testing.T, mode string) (string, syscall.WaitStatus) { + t.Helper() + cmd := exec.Command(os.Args[0]) + cmd.Env = append(os.Environ(), "CRASHHANDLER_TEST_MODE="+mode, "GOTRACEBACK=single") + var stderr strings.Builder + cmd.Stderr = &stderr + err := cmd.Run() + if err == nil { + t.Fatalf("child in mode %q exited cleanly; expected it to die (stderr: %s)", mode, stderr.String()) + } + ee, ok := err.(*exec.ExitError) + if !ok { + t.Fatalf("child in mode %q: unexpected error type %T: %v", mode, err, err) + } + ws, ok := ee.Sys().(syscall.WaitStatus) + if !ok { + t.Fatalf("child in mode %q: no wait status: %v", mode, err) + } + if ws.Exited() && ws.ExitStatus() == 97 { + t.Fatalf("child in mode %q survived its crash trigger (exit 97); handler swallowed the signal? stderr: %s", mode, stderr.String()) + } + return stderr.String(), ws +} + +func TestInstalled(t *testing.T) { + if !Installed() { + t.Fatal("native crash handler not installed at process start") + } +} + +// TestCThreadSegfaultDiesLoudly is the regression test for the production +// incident: a SIGSEGV on a non-Go thread must (a) kill the process instead of +// wedging in the Go runtime's badsignal path, and (b) leave a native backtrace +// on stderr. +func TestCThreadSegfaultDiesLoudly(t *testing.T) { + stderr, ws := runCrashChild(t, "c-thread-segv") + if !ws.Signaled() { + t.Fatalf("child not killed by a signal (status %v); stderr: %s", ws, stderr) + } + if !strings.Contains(stderr, Marker) { + t.Fatalf("stderr missing crash marker %q:\n%s", Marker, stderr) + } + if !strings.Contains(stderr, "SIGSEGV") { + t.Fatalf("stderr does not name the signal:\n%s", stderr) + } + // backtrace_symbols_fd emits one line per frame containing the binary + // path or a hex address; require at least a few frames. + if strings.Count(stderr, "0x") < 3 { + t.Fatalf("stderr does not look like a native backtrace:\n%s", stderr) + } +} + +// TestCgoCallSegfaultDiesLoudly covers a fault inside a cgo call running on a +// Go-created thread (e.g. DuckDB crashing during a query executed via +// duckdb-go). +func TestCgoCallSegfaultDiesLoudly(t *testing.T) { + stderr, ws := runCrashChild(t, "cgo-call-segv") + if ws.Exited() { + t.Fatalf("child exited (%d) instead of dying on a signal; stderr: %s", ws.ExitStatus(), stderr) + } + if !strings.Contains(stderr, Marker) { + t.Fatalf("stderr missing crash marker %q:\n%s", Marker, stderr) + } +} + +// TestGoNilDerefStillPanics pins the chaining contract: the native handler +// must NOT hijack SIGSEGV raised by Go code — those keep producing ordinary +// Go panics with goroutine tracebacks. +func TestGoNilDerefStillPanics(t *testing.T) { + stderr, _ := runCrashChild(t, "go-nil-deref") + if strings.Contains(stderr, Marker) { + t.Fatalf("native handler hijacked a Go nil dereference:\n%s", stderr) + } + if !strings.Contains(stderr, "panic: runtime error") { + t.Fatalf("expected a normal Go panic on stderr:\n%s", stderr) + } +} diff --git a/internal/crashhandler/handler.c b/internal/crashhandler/handler.c new file mode 100644 index 00000000..a1c397ab --- /dev/null +++ b/internal/crashhandler/handler.c @@ -0,0 +1,78 @@ +// Native fatal-signal handler: print a marker + native backtrace to stderr, +// then die with the original signal. Installed from a C constructor so it runs +// BEFORE the Go runtime registers its own handlers — the runtime then saves it +// as the pre-existing handler and forwards non-Go faults to it (sigfwdgo), +// while faults in Go code keep producing ordinary Go panics. +// +// Everything in the handler is async-signal-safe: write(2), backtrace(3), +// backtrace_symbols_fd(3), sigaction(2), raise(3). + +#include +#include +#include +#include +#include + +static volatile bool g_installed = false; + +static void crash_write(const char *s) { + ssize_t unused = write(STDERR_FILENO, s, strlen(s)); + (void)unused; +} + +static const char *crash_signame(int sig) { + switch (sig) { + case SIGSEGV: return "SIGSEGV"; + case SIGBUS: return "SIGBUS"; + case SIGILL: return "SIGILL"; + case SIGFPE: return "SIGFPE"; + case SIGABRT: return "SIGABRT"; + default: return "signal"; + } +} + +static void duckgres_crash_handler(int sig, siginfo_t *info, void *uctx) { + (void)info; + (void)uctx; + + // Keep this prefix in sync with crashhandler.Marker. + crash_write("\nduckgres: fatal native signal "); + crash_write(crash_signame(sig)); + crash_write(" on native thread - backtrace of crashed thread:\n"); + + void *bt[64]; + int n = backtrace(bt, 64); + backtrace_symbols_fd(bt, n, STDERR_FILENO); + + crash_write("duckgres: terminating with default signal disposition\n"); + + // Restore the default disposition and re-raise so the process dies with + // the original signal (and dumps core where enabled) instead of wedging. + struct sigaction dfl; + memset(&dfl, 0, sizeof(dfl)); + dfl.sa_handler = SIG_DFL; + sigemptyset(&dfl.sa_mask); + sigaction(sig, &dfl, NULL); + raise(sig); + // If the signal is blocked during handler execution, returning re-executes + // the faulting instruction and the (now default) action fires then. +} + +__attribute__((constructor)) static void duckgres_crashhandler_install(void) { + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sa.sa_sigaction = duckgres_crash_handler; + // SA_ONSTACK: the Go runtime forwards signals while running on the + // per-thread alternate signal stack; the handler must be willing to run + // there. + sa.sa_flags = SA_SIGINFO | SA_ONSTACK; + sigemptyset(&sa.sa_mask); + + const int sigs[] = {SIGSEGV, SIGBUS, SIGILL, SIGFPE, SIGABRT}; + for (size_t i = 0; i < sizeof(sigs) / sizeof(sigs[0]); i++) { + sigaction(sigs[i], &sa, NULL); + } + g_installed = true; +} + +bool duckgres_crashhandler_installed(void) { return g_installed; } diff --git a/internal/crashhandler/handler_cgo.go b/internal/crashhandler/handler_cgo.go new file mode 100644 index 00000000..e81d9080 --- /dev/null +++ b/internal/crashhandler/handler_cgo.go @@ -0,0 +1,12 @@ +//go:build cgo && (linux || darwin) + +package crashhandler + +/* +#include + +extern bool duckgres_crashhandler_installed(void); +*/ +import "C" + +func installed() bool { return bool(C.duckgres_crashhandler_installed()) } diff --git a/internal/crashhandler/handler_stub.go b/internal/crashhandler/handler_stub.go new file mode 100644 index 00000000..245d2e56 --- /dev/null +++ b/internal/crashhandler/handler_stub.go @@ -0,0 +1,5 @@ +//go:build !cgo || (!linux && !darwin) + +package crashhandler + +func installed() bool { return false } diff --git a/internal/crashhandler/triggers.go b/internal/crashhandler/triggers.go new file mode 100644 index 00000000..e17850bc --- /dev/null +++ b/internal/crashhandler/triggers.go @@ -0,0 +1,42 @@ +//go:build cgo && (linux || darwin) + +package crashhandler + +// Test-only crash triggers. They live in the package (not _test.go files) +// because cgo is not allowed in test files. Nothing in production code calls +// them. + +/* +#include +#include + +static void *duckgres_crashtrigger_thread(void *arg) { + volatile int *p = NULL; + *p = 42; // SIGSEGV on a C-created thread + return NULL; +} + +static void duckgres_crashtrigger_c_thread(void) { + pthread_t t; + pthread_create(&t, NULL, duckgres_crashtrigger_thread, NULL); + pthread_join(t, NULL); +} + +static void duckgres_crashtrigger_direct(void) { + volatile int *p = NULL; + *p = 42; // SIGSEGV inside a cgo call on a Go-created thread +} +*/ +import "C" + +// TriggerCThreadSegfault crashes with SIGSEGV on a thread created by C code +// (never returns). +func TriggerCThreadSegfault() { + C.duckgres_crashtrigger_c_thread() +} + +// TriggerCgoCallSegfault crashes with SIGSEGV inside a cgo call (never +// returns). +func TriggerCgoCallSegfault() { + C.duckgres_crashtrigger_direct() +} diff --git a/main.go b/main.go index bdeb1a83..3d51de36 100644 --- a/main.go +++ b/main.go @@ -17,6 +17,7 @@ import ( "github.com/posthog/duckgres/controlplane" "github.com/posthog/duckgres/duckdbservice" "github.com/posthog/duckgres/internal/cliboot" + "github.com/posthog/duckgres/internal/crashhandler" "github.com/posthog/duckgres/server" ) @@ -55,6 +56,15 @@ func main() { // default SIGPIPE handler is a legacy Unix footgun that kills the process. signal.Ignore(syscall.SIGPIPE) + // The native crash handler self-installs from a C constructor (importing + // the package is what links it in). Without it, a SIGSEGV on a + // DuckDB-created thread can wedge the process forever instead of killing + // it — see the package doc. Surface its state early so a missing handler + // is visible in logs. + if !crashhandler.Installed() { + slog.Warn("Native crash handler NOT installed; fatal signals on native threads may wedge the process.") + } + // Set version on server package so catalog macros can expose it server.SetProcessVersion(version) diff --git a/tests/e2e-mw-dev/README.md b/tests/e2e-mw-dev/README.md index 346589c4..91e2e74f 100644 --- a/tests/e2e-mw-dev/README.md +++ b/tests/e2e-mw-dev/README.md @@ -151,6 +151,15 @@ normal `go test ./...` lane. pooler but not a denied destination needs a stable exec-into-worker probe; deferred (high flake risk). The policies themselves are asserted statically in `tests/manifests/`. +- **Native crash handler (`internal/crashhandler`)** — a SIGSEGV on a + DuckDB-created thread must kill the worker with a native backtrace on stderr + instead of wedging the process (the Go runtime's badsignal path can leave a + crashed C thread's locks held forever). Asserting this in-Job would require + deterministically segfaulting a real worker pod — there is no SQL statement + that does that on purpose. Covered by `internal/crashhandler` package tests, + which re-exec the test binary and crash it on a C-created thread, in a cgo + call, and in pure Go code, asserting death-by-signal + the stderr marker + (and that Go panics stay ordinary panics). - **Oversized Bind-parameter rejection (#717)** — rejecting a Bind message whose declared parameter length exceeds the remaining message body requires crafting a malformed wire-protocol packet on a raw socket (through TLS + auth); libpq From f98ffdfc3476faede4e164bf375ed55e420ace56 Mon Sep 17 00:00:00 2001 From: Benjamin Knofe-Vider Date: Fri, 3 Jul 2026 12:35:58 +0200 Subject: [PATCH 2/2] crashhandler: report pc + fault addr, portable test assertions glibc's backtrace() through a signal frame can yield as few as two frames on linux/amd64, which failed the test's frame-count assertion in CI (the handler itself worked: marker + death by signal). Assert one frame address plus the fault-address line instead, matching both the glibc and macOS backtrace_symbols_fd formats. Make the shallow-trace case actionable: the report line now includes the faulting PC (from the signal ucontext on linux/darwin amd64+arm64) and si_addr, printed with an async-signal-safe hex writer, so a crash site can be resolved with addr2line even without deep unwind. --- internal/crashhandler/crashhandler_test.go | 15 ++++-- internal/crashhandler/handler.c | 59 ++++++++++++++++++++-- 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/internal/crashhandler/crashhandler_test.go b/internal/crashhandler/crashhandler_test.go index 0c717536..68135b1b 100644 --- a/internal/crashhandler/crashhandler_test.go +++ b/internal/crashhandler/crashhandler_test.go @@ -3,6 +3,7 @@ package crashhandler import ( "os" "os/exec" + "regexp" "strings" "syscall" "testing" @@ -80,10 +81,16 @@ func TestCThreadSegfaultDiesLoudly(t *testing.T) { if !strings.Contains(stderr, "SIGSEGV") { t.Fatalf("stderr does not name the signal:\n%s", stderr) } - // backtrace_symbols_fd emits one line per frame containing the binary - // path or a hex address; require at least a few frames. - if strings.Count(stderr, "0x") < 3 { - t.Fatalf("stderr does not look like a native backtrace:\n%s", stderr) + // backtrace_symbols_fd's frame format differs per platform + // ("binary[0xADDR]" on glibc, "N binary 0xADDR symbol + off" on macOS) + // and the unwind depth through a signal frame varies (glibc on + // linux/amd64 can produce as few as two frames), so require just one + // frame address plus the fault-address line the handler prints itself. + if !regexp.MustCompile(`0x[0-9a-f]{4,}`).MatchString(stderr) { + t.Fatalf("stderr does not contain a backtrace frame address:\n%s", stderr) + } + if !strings.Contains(stderr, "fault addr 0x") { + t.Fatalf("stderr does not report the fault address:\n%s", stderr) } } diff --git a/internal/crashhandler/handler.c b/internal/crashhandler/handler.c index a1c397ab..a0aa9232 100644 --- a/internal/crashhandler/handler.c +++ b/internal/crashhandler/handler.c @@ -7,10 +7,20 @@ // Everything in the handler is async-signal-safe: write(2), backtrace(3), // backtrace_symbols_fd(3), sigaction(2), raise(3). +#ifdef __linux__ +#define _GNU_SOURCE // REG_RIP in +#endif +#ifdef __APPLE__ +#define _XOPEN_SOURCE 700 // ucontext_t register access +#define _DARWIN_C_SOURCE // keep SA_ONSTACK etc visible alongside _XOPEN_SOURCE +#endif + #include #include #include +#include #include +#include #include static volatile bool g_installed = false; @@ -20,6 +30,46 @@ static void crash_write(const char *s) { (void)unused; } +static void crash_write_hex(uintptr_t v) { + char buf[2 + 2 * sizeof(v)]; + size_t n = 0; + buf[n++] = '0'; + buf[n++] = 'x'; + bool started = false; + for (int shift = 8 * (int)sizeof(v) - 4; shift >= 0; shift -= 4) { + unsigned d = (unsigned)((v >> shift) & 0xf); + if (!started && d == 0 && shift != 0) { + continue; + } + started = true; + buf[n++] = "0123456789abcdef"[d]; + } + ssize_t unused = write(STDERR_FILENO, buf, n); + (void)unused; +} + +// crash_pc extracts the faulting instruction pointer from the signal context +// where we know the layout; 0 elsewhere. Even a shallow backtrace plus this PC +// is enough to addr2line the crash site. +static uintptr_t crash_pc(void *uctx) { + if (uctx == NULL) { + return 0; + } + ucontext_t *uc = (ucontext_t *)uctx; +#if defined(__linux__) && defined(__x86_64__) + return (uintptr_t)uc->uc_mcontext.gregs[REG_RIP]; +#elif defined(__linux__) && defined(__aarch64__) + return (uintptr_t)uc->uc_mcontext.pc; +#elif defined(__APPLE__) && defined(__aarch64__) + return (uintptr_t)uc->uc_mcontext->__ss.__pc; +#elif defined(__APPLE__) && defined(__x86_64__) + return (uintptr_t)uc->uc_mcontext->__ss.__rip; +#else + (void)uc; + return 0; +#endif +} + static const char *crash_signame(int sig) { switch (sig) { case SIGSEGV: return "SIGSEGV"; @@ -32,13 +82,14 @@ static const char *crash_signame(int sig) { } static void duckgres_crash_handler(int sig, siginfo_t *info, void *uctx) { - (void)info; - (void)uctx; - // Keep this prefix in sync with crashhandler.Marker. crash_write("\nduckgres: fatal native signal "); crash_write(crash_signame(sig)); - crash_write(" on native thread - backtrace of crashed thread:\n"); + crash_write(" pc "); + crash_write_hex(crash_pc(uctx)); + crash_write(" fault addr "); + crash_write_hex(info != NULL ? (uintptr_t)info->si_addr : 0); + crash_write(" - backtrace of crashed thread:\n"); void *bt[64]; int n = backtrace(bt, 64);