unikraft / lib-musl

musl: A C standard library
Other
9 stars 29 forks source link

Ignoring signals fails #57

Closed mkroening closed 1 year ago

mkroening commented 1 year ago

On Linux, when the Rust runtime initializes, it resets the SIGPIPE handler to ignore. (std/src/sys/unix/mod.rs#L59-L67, std/src/sys/unix/mod.rs#L164-L195):

    // By default, some platforms will send a *signal* when an EPIPE error
    // would otherwise be delivered. This runtime doesn't install a SIGPIPE
    // handler, causing it to kill the program, which isn't exactly what we
    // want!
    //
    // Hence, we set SIGPIPE to ignore when the program starts up in order
    // to prevent this problem. Add `#[unix_sigpipe = "..."]` above `fn main()` to
    // alter this behavior.

What Rust does is equivalent to the following C program:

#include <assert.h>
#include <signal.h>

int main(int argc, char *argv[]) {
    void (*res)(int) = signal(SIGPIPE, SIG_IGN);
    assert(res != SIG_ERR);

    return 0;
}
[...]
Assertion failed: res != SIG_ERR (/home/kroening/devel/unikraft/apps/app-helloworld/main.c: main: 6)
[    0.105735] CRIT: [libmusl] <abort.c @    7> abort called

Returning a success value without doing anything would resolve this problem.

eduardvintila commented 1 year ago

Hey, @mkroening. I have looked into this issue and found out the problem lies in a mismatch between sigaction structs of Unikraft and the libc ones. Fortunately, we already have @andreittr's PR which solves this problem 👍🏻 Please give it a try and let us know if this issue still persists - I've tried it myself on your example and it works.

mkroening commented 1 year ago

Hi @eduardvintila, thanks for looking into this! :)

Unfortunately, this does not solve the issue for me. I can still produce the issue with the C code above. Can you reproduce the issue with an unpatched Unikraft?

My Kraftfile looks as follows:

specification: v0.5

unikraft:
  version: stable

targets:
  - name: default
    architecture: x86_64
    platform: kvm

libraries:
  musl: stable

Before and after applying the patch and rebuilding with --no-pull, signal returns SIG_ERR.

Note that I circumvented the issue for now—at least for Rust startup. I opted out of using signal for Unikraft (https://github.com/rust-lang/rust/pull/113411/commits/9ef25729e7ce6a516e2494ad91ef91101dd3b4b1), but it would be great if we would not have to.

eduardvintila commented 1 year ago

Hey, @mkroening! Sorry for taking so much time to get back with a response...

The PR I have mentioned is now upstream. Please try to run everything while on the staging (not stable) branch. Do you have the uksignal library selected? You must include it in order for your example to work - the issue is indeed present when it's not enabled :)

mkroening commented 1 year ago

Thanks for your reply! I can confirm it works.

The hint with uksignal was especially helpful. I found it surprising that CONFIG_LIBMUSL_SIGNAL does not enable CONFIG_LIBUKSIGNAL, but you instead have to do that manually.

Do you think that behavior should be changed, and I should open an issue, or is this intended?

eduardvintila commented 1 year ago

@mkroening, you make a very good point - probably the reason why this was not the case is that uksignal used to be broken a few releases back. Even though we still don't have a complete implementation, it's functional now. No need for an issue, I think. I'll directly make a PR on this. Thank you!