Swift crash handler reads si_addr for signals where it is not guaranteed to be filled

[rjmansfield]

Description

The crash handler will report the program faulted at si_addr, however si_addr is not filled for all signals, for example, SIGQUIT, SIGABORT. This leads to reporting a misleading message where the address is not valid.

crashInfo.fault_address = (uint64_t)pinfo->si_addr;

According to POSIX si_addr may be filled for SIGILL, SIGFPE, SIGSEGV and SIGBUS

https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/signal.h.html

For some implementations, the value of si_addr may be inaccurate.

Reproduction

$ cat abort.swift 
import Glibc

abort()
$ ./abort

💣 Program crashed: Aborted at 0x3a07e15300047ea1

Thread 0 "abort" crashed:

0 0x00007f436bb4b9fc pthread_kill@@GLIBC_2.34 + 300 in libc.so.6

where 0x3a07e15300047ea1 is not a valid instruction.

Expected behavior

Not printing a misleading fault address. Perhaps the pc could be read from the ucontext

Environment

$ swiftc -v
Swift version 6.2-dev (LLVM 2e7485f44847d34, Swift 1e96466)

Additional information

No response

[al45tair]

FWIW, the text about si_addr being inaccurate is, I think, a reference to the fact that some architectures have inexact exceptions, hence the address in it might point at the wrong instruction.

The real issue you have is that your libc.so.6 was built without frame pointers, which means you aren't getting a useful backtrace (which would point at the correct place). The program counter is already visible — it's 0x00007f436bb4b9fc.

Looking at the documentation for sigaction on Linux (which is the relevant documentation here — POSIX can say whatever it wants; Linux doesn't necessarily follow it to the letter), it does look like Linux only sets si_addr for a handful of signals, not including SIGABRT, which is a shame because it would make sense to set it for that one too.

We should probably display the program counter for SIGABRT traps; I'll check the other signals we catch to make sure that we do something sensible for those also.

[rjmansfield]

The real issue you have is that your libc.so.6 was built without frame pointers, which means you aren't getting a useful backtrace (which would point at the correct place). The program counter is already visible — it's 0x00007f436bb4b9fc.

Sure, but that's not the issue being reported here. Even if I had a libc built with frame pointers, it would still incorrectly report a garbage fault address. I'm not confused as to where the fault occurred, but as I mentioned, the bogus fault address in the message may be confusing to some users.

Looking at the documentation for sigaction on Linux (which is the relevant documentation here — POSIX can say whatever it wants; Linux doesn't necessarily follow it to the letter), it does look like Linux only sets si_addr for a handful of signals, not including SIGABRT, which is a shame because it would make sense to set it for that one too.

My point was that POSIX only requires implementations to provide si_addr for those four signals. Linux is conforming in this regard and only filling si_addr for a subset of POSIX signals. i.e. according to POSIX, the Linux behaviour of not filling it in for SIGABRT is valid. Regarding, the linux sigaction manpage, it explicitly states its intention to conform to POSIX.

CONFORMING TO
       POSIX.1-2001, POSIX.1-2008, SVr4.

[al45tair]

@rjmansfield Interestingly, when I tried to reproduce this locally, SIGABRT and SIGQUIT both seemed to set the si_addr field (which is totally allowed by POSIX — it just says when you must set it, not that you can't set it for other things). Which distro are you using here exactly?

(I have a fix, but I'd like to see it fail and then see it work with the fix, where right now I just see it working all the time…)

[rjmansfield]

I was observing this with Ubuntu 22.04 w/ 5.15.0-136-generic on x86_64

[al45tair]

I was observing this with Ubuntu 22.04 w/ 5.15.0-136-generic on x86_64

Weird. On Ubuntu 22.04 6.8.0-52-generic on x86_64, I don't see any problem. So maybe things have changed and Linux is now filling in this field for SIGABRT and SIGQUIT?