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

Unresponsive to SIGTERM. #40

Closed
cedws opened this issue Feb 14, 2019 · 14 comments
Closed

Unresponsive to SIGTERM. #40

cedws opened this issue Feb 14, 2019 · 14 comments

Comments

@cedws
Copy link

cedws commented Feb 14, 2019

  • Run Cage in a terminal
  • Observe that Ctrl+C has no effect

Cage should clean up and exit on SIGTERM if possible.

@Hjdskes
Copy link
Collaborator

Hjdskes commented Feb 14, 2019

This works for me and is implemented: https://github.com/Hjdskes/cage/blob/master/cage.c#L92.

Sometimes there's a delay, but I suppose that is the application within Cage shutting down (or crashing), since the display terminates. We should probably properly shut down the applications within Cage, if this is possible. What application are you noticing this with?

@cedws
Copy link
Author

cedws commented Feb 14, 2019

firefox-wayland. I gave it about 10 seconds or so but it seems to ignore my requests.

image

@cedws
Copy link
Author

cedws commented Feb 14, 2019

static int
handle_signal(int signal, void *data)
{
	struct wl_display *display = data;

	switch (signal) {
	case SIGINT:
		/* Fallthrough */
	case SIGTERM:
		wl_display_terminate(display);
+		exit(0);
-		return 0;
	default:
		return 1;
	}
}

This fixes it for me, but I suppose the int returned from this function is supposed to be used by Wayland as an exit code anyway, right?

@Hjdskes
Copy link
Collaborator

Hjdskes commented Feb 16, 2019

This also makes Firefox close instantly for me:

diff --git a/cage.c b/cage.c
index 7f5b6f4..cf8c71f 100644
--- a/cage.c
+++ b/cage.c
@@ -48,6 +48,9 @@ spawn_primary_client(char *argv[], pid_t *pid_out)
 {
        pid_t pid = fork();
        if (pid == 0) {
+               sigset_t set;
+               sigemptyset(&set);
+               sigprocmask(SIG_SETMASK, &set, NULL);
                execvp(argv[0], argv);
                _exit(1);
        } else if (pid == -1) {

However, Cage does segfault on close now. Still looking into that, but it'd be great if you could test the above.

@Hjdskes
Copy link
Collaborator

Hjdskes commented Feb 16, 2019

However, Cage does segfault on close now.

Scratch that, it did that before. If the above works for you, I'll get that in and open a new issue for the segfault.

@Hjdskes Hjdskes added this to the v0.1 milestone Feb 16, 2019
@cedws
Copy link
Author

cedws commented Feb 16, 2019

The patch works 👍. As you say, there is a segfault on close, but it isn't critical.

@Hjdskes
Copy link
Collaborator

Hjdskes commented Feb 16, 2019

Great! I'll push the fix tomorrow.

@cedws
Copy link
Author

cedws commented Feb 23, 2019

I've just noticed this is still happening if an application isn't passed. Same unresponsiveness to SIGTERM.

@Hjdskes
Copy link
Collaborator

Hjdskes commented Feb 24, 2019

Can you elaborate? What do you mean when you say "an application isn't passed?"

When I launch Cage with Termite and from Termite launch Epiphany, then CTRL-C Cage, everything closes immediately.

@Hjdskes Hjdskes reopened this Feb 24, 2019
@Hjdskes Hjdskes removed this from the v0.1 milestone Feb 24, 2019
@cedws
Copy link
Author

cedws commented Feb 24, 2019

master currently isn't building with the latest version of wlroots so I can't retest. I mean just running cage with no parameters.

@Hjdskes
Copy link
Collaborator

Hjdskes commented Feb 24, 2019

For what it's worth, personally I keep wlroots at 0.3 and develop against that. It's enough work to develop Cage that I'm not wanting to constantly keep up with every breaking change in wlroots :)

@Hjdskes
Copy link
Collaborator

Hjdskes commented Feb 24, 2019

Hm.. you're not supposed to be able to run cage without any application to start. I'll look into that.

@cedws
Copy link
Author

cedws commented Feb 24, 2019

I personally prefer sticking to tagged releases of wlroots, but most projects don't seem to do that. I've built master now and it is still an issue.

@Hjdskes
Copy link
Collaborator

Hjdskes commented Feb 26, 2019

This is fixed in 848929c.

@Hjdskes Hjdskes closed this as completed Feb 26, 2019
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

2 participants