Stream: git-wasmtime

Topic: wasmtime / issue #9396 s390x: Crash on accessing backchai...

Wasmtime GitHub notifications bot (Oct 08 2024 at 16:01):

uweigand opened issue #9396:

When running tests/host_segfault.rs natively on s390x (these tests are skipped in the QEMU based CI), the following sub-tests fail with an unexpected SIGSEGV:

            "overrun 8k with misconfigured host"
            "overrun 32k with misconfigured host"

These tests verify the correct behavior after a segmentation fault due to accessing the guard page after a stack overrun. That actually all works correctly on s390x, but then the signal handler itself crashes. This is because the trap_handler does

        let faulting_addr = match signum {
            libc::SIGSEGV | libc::SIGBUS => Some((*siginfo).si_addr() as usize),
            _ => None,
        };
        let regs = get_trap_registers(context, signum);
        let test = info.test_if_trap(regs, faulting_addr, |handler| {
            handler(signum, siginfo, context)
        });

        // Figure out what to do based on the result of this handling of
        // the trap. Note that our sentinel value of 1 means that the
        // exception was handled by a custom exception handler, so we
        // keep executing.
        let jmp_buf = match test {
            TrapTest::NotWasm => {
                if let Some(faulting_addr) = faulting_addr {
                    let start = info.async_guard_range.start;
                    let end = info.async_guard_range.end;
                    if start as usize <= faulting_addr && faulting_addr < end as usize {
                        abort_stack_overflow();
                    }
                }
                return false;
            }
            TrapTest::HandledByEmbedder => return true,
            TrapTest::Trap { jmp_buf } => jmp_buf,
        };

Note that it calls get_trap_registers before checking for the stack overflow case. That works on other platforms, but on s390x this routine actually accesses the backchain value (our equivalent to the frame pointer used for stack unwinding) - and this is not stored in a register but in the bottom-most word on the stack:

        } else if #[cfg(all(target_os = "linux", target_arch = "s390x"))] {
            // On s390x, SIGILL and SIGFPE are delivered with the PSW address
            // pointing *after* the faulting instruction, while SIGSEGV and
            // SIGBUS are delivered with the PSW address pointing *to* the
            // faulting instruction.  To handle this, the code generator registers
            // any trap that results in one of "late" signals on the last byte
            // of the instruction, and any trap that results in one of the "early"
            // signals on the first byte of the instruction (as usual).  This
            // means we simply need to decrement the reported PSW address by
            // one in the case of a "late" signal here to ensure we always
            // correctly find the associated trap handler.
            let trap_offset = match _signum {
                libc::SIGILL | libc::SIGFPE => 1,
                _ => 0,
            };
            let cx = &*(cx as *const libc::ucontext_t);
            TrapRegisters {
                pc: (cx.uc_mcontext.psw.addr - trap_offset) as usize,
                fp: *(cx.uc_mcontext.gregs[15] as *const usize),
            }

As the stack pointer at the point of fault points to the guard page, that last pointer dereference will also trigger a SIGSEGV. That causes the trap_handler to be entered recursively, and it then aborts.

I guess there would be a number of options to fix this:

• move the guard page check to before calling get_trap_registers - not sure if this causes any semantical difference?
• try to move accessing the backchain until after we've determined this is a NotWasm case (we only need pc to determine this, not fp) - that should be completely equivalent semantically, but would require refactoring of the test_if_trap logic
• handle the special case of the recursive SIGSEGV on accessing the backchain somehow - this would likely require some flag to be set while attempting the access that the recursive call to trap_handler would check

@alexcrichton you've been working on this part of the code recently - any opinions or suggestions?

Wasmtime GitHub notifications bot (Oct 08 2024 at 17:46):

alexcrichton commented on issue #9396:

Hm this one's tricky... The guard page checks won't actually do anything here, that's only for async stack pages and isn't relevant for overflows on host-managed stacks (like the main thread) which these tests are exercising. In that sense they wouldn't actually hit the abort there, so moving that check wouldn't help.

I'm also not a fan of recursive signal handlers, so I'd prefer to avoid that if possible (hard to reason about).

I like your idea of refactoring how the backchain is accessed, so I think that's the way to go. Perhaps something like a closure to pass in of "call this to get the frame pointer" and that is only called once we're sure it's a wasm fault.

Wasmtime GitHub notifications bot (Oct 08 2024 at 21:25):

uweigand commented on issue #9396:

The closure seems to work; I don't get the recursive segfault any more. However, the test case still fails because - as you say - the guard page check doesn't hit, so the original segfault is still delivered after all.

I'm now confused how this test is even supposed to work at all. The JITed stack checks don't hit as the wasm stack limit is deliberately misconfigured, and the guard page check doesn't hit as it's not an async stack. How is this supposed to be detected as stack overflow vs. random segfault?

Wasmtime GitHub notifications bot (Oct 08 2024 at 22:50):

alexcrichton commented on issue #9396:

Oh for these tests Wasmtime shouldn't actually do anything. In theory the Wasmtime signal handler should defer to the previous signal handler, in this case the Rust standard library. The Rust standard library should have metadata about the guard page for the current thread and should be able to figure out that the guard page was hit, and then libstd is the one that prints the message and aborts.

Or at least that's what's supposed to happen (IIRC), maybe Rust's s390x libstd doesn't have the handler installed or doesn't have the support for detecting the current guard page? IIRC that was a pretty gnarly platform-specific part of libstd. You can test it out locally by writing a program that just overflows the stack. If nothing is printed then these tests just aren't destined to pass on s390x, but if a message is printed then they should be passing.

Wasmtime GitHub notifications bot (Oct 08 2024 at 23:35):

uweigand commented on issue #9396:

Ah, I see. However, Rust's libstd also seems to be working correctly. It checks the fault address against the guard page range, but this doesn't hit either as the guard page is just 4k, and the test case allocates 8k per stack frame, so the stack pointer jumps from above the guard to below the guard. Is the JITed code somehow supposed to be doing per-page probing here?

Wasmtime GitHub notifications bot (Oct 08 2024 at 23:40):

uweigand commented on issue #9396:

Indeed, this seems to be done on other architectures, but not s390x:

pub(crate) fn probestack_supported(arch: Architecture) -> bool {
    matches!(
        arch,
        Architecture::X86_64 | Architecture::Aarch64(_) | Architecture::Riscv64(_)
    )
}

I guess I need to implement probestack then ...

Wasmtime GitHub notifications bot (Oct 08 2024 at 23:46):

alexcrichton commented on issue #9396:

Ah yes indeed I forgot about that! That's what we use for guaranteeing the guard page is hit yeah

Wasmtime GitHub notifications bot (Oct 09 2024 at 19:28):

alexcrichton closed issue #9396:

When running tests/host_segfault.rs natively on s390x (these tests are skipped in the QEMU based CI), the following sub-tests fail with an unexpected SIGSEGV:

            "overrun 8k with misconfigured host"
            "overrun 32k with misconfigured host"

These tests verify the correct behavior after a segmentation fault due to accessing the guard page after a stack overrun. That actually all works correctly on s390x, but then the signal handler itself crashes. This is because the trap_handler does

        let faulting_addr = match signum {
            libc::SIGSEGV | libc::SIGBUS => Some((*siginfo).si_addr() as usize),
            _ => None,
        };
        let regs = get_trap_registers(context, signum);
        let test = info.test_if_trap(regs, faulting_addr, |handler| {
            handler(signum, siginfo, context)
        });

        // Figure out what to do based on the result of this handling of
        // the trap. Note that our sentinel value of 1 means that the
        // exception was handled by a custom exception handler, so we
        // keep executing.
        let jmp_buf = match test {
            TrapTest::NotWasm => {
                if let Some(faulting_addr) = faulting_addr {
                    let start = info.async_guard_range.start;
                    let end = info.async_guard_range.end;
                    if start as usize <= faulting_addr && faulting_addr < end as usize {
                        abort_stack_overflow();
                    }
                }
                return false;
            }
            TrapTest::HandledByEmbedder => return true,
            TrapTest::Trap { jmp_buf } => jmp_buf,
        };

Note that it calls get_trap_registers before checking for the stack overflow case. That works on other platforms, but on s390x this routine actually accesses the backchain value (our equivalent to the frame pointer used for stack unwinding) - and this is not stored in a register but in the bottom-most word on the stack:

        } else if #[cfg(all(target_os = "linux", target_arch = "s390x"))] {
            // On s390x, SIGILL and SIGFPE are delivered with the PSW address
            // pointing *after* the faulting instruction, while SIGSEGV and
            // SIGBUS are delivered with the PSW address pointing *to* the
            // faulting instruction.  To handle this, the code generator registers
            // any trap that results in one of "late" signals on the last byte
            // of the instruction, and any trap that results in one of the "early"
            // signals on the first byte of the instruction (as usual).  This
            // means we simply need to decrement the reported PSW address by
            // one in the case of a "late" signal here to ensure we always
            // correctly find the associated trap handler.
            let trap_offset = match _signum {
                libc::SIGILL | libc::SIGFPE => 1,
                _ => 0,
            };
            let cx = &*(cx as *const libc::ucontext_t);
            TrapRegisters {
                pc: (cx.uc_mcontext.psw.addr - trap_offset) as usize,
                fp: *(cx.uc_mcontext.gregs[15] as *const usize),
            }

As the stack pointer at the point of fault points to the guard page, that last pointer dereference will also trigger a SIGSEGV. That causes the trap_handler to be entered recursively, and it then aborts.

I guess there would be a number of options to fix this:

• move the guard page check to before calling get_trap_registers - not sure if this causes any semantical difference?
• try to move accessing the backchain until after we've determined this is a NotWasm case (we only need pc to determine this, not fp) - that should be completely equivalent semantically, but would require refactoring of the test_if_trap logic
• handle the special case of the recursive SIGSEGV on accessing the backchain somehow - this would likely require some flag to be set while attempting the access that the recursive call to trap_handler would check

@alexcrichton you've been working on this part of the code recently - any opinions or suggestions?

Last updated: Nov 22 2024 at 16:03 UTC