Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macos: restoring signal disposition doesn't work as flags are not propagated #4299

Open
santigimeno opened this issue Feb 5, 2024 · 9 comments

Comments

@santigimeno
Copy link
Member

Take this very basic example:

#include <stdio.h>
#include <signal.h>
#include <stdlib.h>

void handler(int signum) {
    printf("Signal handler called with signal %d\n", signum);
}

int main() {
    struct sigaction sa, old_sa;

    // Set a signal handler with SA_RESETHAND flag
    sa.sa_handler = handler;
    sigemptyset(&sa.sa_mask);
    sa.sa_flags = SA_RESETHAND;
    if (sigaction(SIGINT, &sa, NULL) == -1) {
        perror("sigaction");
        exit(1);
    }

    // Set a second signal handler without SA_RESETHAND flag
    sa.sa_flags = 0;
    if (sigaction(SIGINT, &sa, &old_sa) == -1) {
        perror("sigaction");
        exit(1);
    }

    // Check whether the SA_RESETHAND flag is actually propagated
    if (old_sa.sa_flags & SA_RESETHAND) {
        printf("SA_RESETHAND was set in the old signal handler\n");
    } else {
        printf("SA_RESETHAND was not set in the old signal handler\n");
    }

    return 0;
}

Running this on linux produces the correct output:

$ SA_RESETHAND was set in the old signal handler

Whereas on macos (arm64) it doesn't:

$ SA_RESETHAND was not set in the old signal handler

I observed this while running current v1.x with the node.js test suite and caused some signal tests to fail. They started failing after b9421d7, though I don't know if there's much we can do from the libuv side as it seems a macos issue. Thoughts?

@vtjnash
Copy link
Member

vtjnash commented Feb 5, 2024

There appear to be some mistakes in that commit, which don't entirely negate the fact this appears to be a kernel bug, but is making it work poorly. In particular

static void uv__sigaction_set(int signum, struct sigaction *sa) {
  uv__sigactions.acts[signum] = *sa;
  uv__sigactions.acts_presented_flags[signum] = 1;
}

looks like it is missing a conditional set:

static void uv__sigaction_set(int signum, struct sigaction *sa) {
  if (uv__sigactions.acts_presented_flags[signum] == 0)
    uv__sigactions.acts[signum] = *sa;
  uv__sigactions.acts_presented_flags[signum] = 1;
}

since libuv may re-register its own sigaction to change from SA_RESETHAND => 0, and that would overwrite the correct info for the old sigaction, leading to libuv not correctly unregistering itself later

@bnoordhuis
Copy link
Member

@slavamuravey fyi

I don't know if there's much we can do from the libuv side as it seems a macos issue

Yes... Guess there isn't much we can do here except maybe add a regression test to ensure we don't forget this tidbit of macos lore and merge another attempt in the future.

@slavamuravey
Copy link
Contributor

slavamuravey commented Mar 6, 2024

@bnoordhuis @vtjnash @santigimeno
Perhaps we should store previous sigaction data ourselves and not rely on the kernel?
Here is an example based on @santigimeno's example:

#define _GNU_SOURCE
#include <stdio.h>
#include <signal.h>
#include <stdlib.h>

void handler(int signum) {
    printf("Signal handler called with signal %d\n", signum);
}

int main() {
    struct sigaction sa, old_sa;

    // Set a signal handler with SA_RESETHAND flag
    sa.sa_handler = handler;
    sigemptyset(&sa.sa_mask);
    sa.sa_flags = SA_RESETHAND;
    if (sigaction(SIGINT, &sa, NULL) == -1) {
        perror("sigaction");
        exit(1);
    }
    // Here we remember previous sigaction data
    old_sa = sa;

    // Set a second signal handler without SA_RESETHAND flag
    sa.sa_flags = 0;
    if (sigaction(SIGINT, &sa, NULL) == -1) {
        perror("sigaction");
        exit(1);
    }

    // Check whether the SA_RESETHAND flag is actually propagated
    if (old_sa.sa_flags & SA_RESETHAND) {
        printf("SA_RESETHAND was set in the old signal handler\n");
    } else {
        printf("SA_RESETHAND was not set in the old signal handler\n");
    }

    return 0;
}

@bnoordhuis
Copy link
Member

Perhaps we should store previous sigaction data ourselves and not rely on the kernel?

I don't think that would work reliably. A third party (like another library) may have changed the signal handler before libuv gets to it.

Longstanding libuv philosophy is that features should either work always or never, not sometimes. Users can code defensively around the first two but not the last one.

@slavamuravey
Copy link
Contributor

@bnoordhuis Ok, then we should wait for changes in macos. After macos behavior will be the same as in linux for our case, we will be able to merge reverted PR again?

@bnoordhuis
Copy link
Member

Yes, but that may be a while because with macos we generally support the latest-1 or latest-2 releases (e.g. macos 11 and up right now.)

@slavamuravey
Copy link
Contributor

Got it, thanks!

@slavamuravey
Copy link
Contributor

@bnoordhuis What do you think, what if we apply changes from reverted PR for everything except of macos? And when issues will be fixed in macos, we apply changes for it too.

@bnoordhuis
Copy link
Member

Libuv's goal is to abstract away platform differences. If something can't be made to work in a cross-platform way, we don't do it.

macOS (and Linux and Windows) are popular enough that we can't just shrug and say "too bad, doesn't work here."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants