Reset SIGCHLD action to SIG_DFL

[joelpelaez]

Some applications, like Erlang, call sigaction for SIGCHLD with sa.sa_action = SIG_IGN, this causes bwrap never read child process exit status from signalfd.

That behavior is explained in the function do_notify_parent at kernel/signal.c from linux kernel source, to be exact, in this section evaluates the signal action value:

https://github.com/torvalds/linux/blob/284922f4c563aa3a8558a00f2a05722133237fe8/kernel/signal.c#L2232-L2253

This fixes #705

[gamax92]

This bug is also affecting running volumeicon with the new glycin based loader for gdk-pixbuf. Without the patch, bwrap hangs in monitor_child, and starts up correctly with.

[smcv]

This seems more like a workaround than a solution: applications can't just assume that every subprocess that they might run can cope with functionally-significant signals being blocked.

[joelpelaez]

This seems more like a workaround than a solution: applications can't just assume that every subprocess that they might run can cope with functionally-significant signals being blocked.

Some applications like Erlang sets SIGCHLD to SIG_IGN because "recognized" children process are fork'ed using a helper child process, but this issue is related to an "unrecognized" child process started by an external library of an plugin, in this example is because Erlang has a wxWidgets graphical wrapper that uses GTK and it uses glycin for image loading.

I think that if an program has to manage children processes it must set the necessary signal settings for work properly.

[smcv]

I think that if an program has to manage children processes it must set the necessary signal settings for work properly.

Sure, but this reasoning applies equally to Erlang: it can't just break normal process management and assume that everything will be OK.

[reneleonhardt]

Hello @joelpelaez thank you very much for your work! Can it be reviewed again? smoke-test failed last time.

[joelpelaez]

Hello @joelpelaez thank you very much for your work! Can it be reviewed again? smoke-test failed last time.

The error with the smoke-test is from main branch, I don't understand why it fails. My change only resets SIGCHLD signal settings, it wouldn't change the program behavior.

[reneleonhardt]

The error with the smoke-test is from main branch, I don't understand why it fails. My change only resets SIGCHLD signal settings, it wouldn't change the program behavior.

I found it, it's caused by the update to ubuntu:24.04.
https://github.com/containers/bubblewrap/actions/workflows/check.yml
ubuntu-latest was changed a year ago.
https://github.com/actions/runner-images/commits/main/README.md
uid_map can't be written to.
https://github.com/containers/bubblewrap/blob/main/bubblewrap.c#L996

if (write_file_at (dir_fd, "uid_map", uid_map) != 0)
    die_with_error ("setting up uid map");

[joelpelaez]

The error with the smoke-test is from main branch, I don't understand why it fails. My change only resets SIGCHLD signal settings, it wouldn't change the program behavior.

I found it, it's caused by the update to ubuntu:24.04. https://github.com/containers/bubblewrap/actions/workflows/check.yml ubuntu-latest was changed a year ago. https://github.com/actions/runner-images/commits/main/README.md uid_map can't be written to. https://github.com/containers/bubblewrap/blob/main/bubblewrap.c#L996

if (write_file_at (dir_fd, "uid_map", uid_map) != 0)
    die_with_error ("setting up uid map");

I did a separated PR with the changes for re-enable user namespaces in CI (#728)

[smcv]

I did a separated PR with the changes for re-enable user namespaces in CI (#728)

Merged, please rebase.

[smcv]

While rebasing, please squash your three commits into one "fully correct" commit.

[smcv]

Thanks! Merging.

[reneleonhardt]

While rebasing, please squash your three commits into one "fully correct" commit.

Out of curiosity, wouldn't it be easier to enable squashing when merging?
https://github.blog/open-source/git/squash-your-commits/
https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests

So maintainers can squash themselves if they want and don't have to wait for contributors, could be faster 😄

[smcv]

Out of curiosity, wouldn't it be easier to enable squashing when merging?

It's already allowed, but if the PR submitter needs to rebase the patch series anyway to get CI to pass, then they might as well make the patch series "correct" at the same time.

And, in general, automatically squashing commits during merge is a lossy operation: non-trivial patch serieses often intentionally have more than one commit (like the common pattern "refactor; refactor more; add new feature that makes use of the refactorings"). This particular change was sufficiently simple that one commit was enough, but not all PRs fall into that category.

This particular project also has a policy of requiring Signed-off-by for a copyright/licensing paper trail, and I'm not sure whether squashing commits preserves that.