Skip to content

Fix spawn_pane() with newer libevent that uses signalfd#3621

Closed
azat wants to merge 1 commit intotmux:masterfrom
azat-archive:libevent-signalfd-fix
Closed

Fix spawn_pane() with newer libevent that uses signalfd#3621
azat wants to merge 1 commit intotmux:masterfrom
azat-archive:libevent-signalfd-fix

Conversation

@azat
Copy link
Copy Markdown

@azat azat commented Jul 9, 2023

There is closefrom() and after it calls proc_clear_signals() which in turn tries to close the created signalfd and fails:

Assertion ret == 0 failed in sigfd_free_sigevent

Fix this by reorder the calls.

Fixes: #3572
Fixes: libevent/libevent#1460

THere is closefrom() and after it calls proc_clear_signals() which in
turn tries to close the created signalfd and fails:

    Assertion ret == 0 failed in sigfd_free_sigevent

Fix this by reorder the calls.
@nicm
Copy link
Copy Markdown
Member

nicm commented Jul 9, 2023

This looks OK, I will apply it tomorrow, thanks. It is not a great idea to assert if close fails however.

@nicm
Copy link
Copy Markdown
Member

nicm commented Jul 9, 2023

I've applied this to OpenBSD now, it will be in GitHub later, thanks! I did name you in the commit but I forget to mention the issue number.

@nicm nicm closed this Jul 9, 2023
@azat azat deleted the libevent-signalfd-fix branch July 10, 2023 05:45
@azat
Copy link
Copy Markdown
Author

azat commented Jul 10, 2023

It is not a great idea to assert if close fails however.

I thought about this, but decided to stick with assert for now, since this is for debug builds only, that should be used only by developers (who possibly could dig and fix such issues)

I've applied this to OpenBSD now, it will be in GitHub later, thanks! I did name you in the commit but I forget to mention the issue number.

Thanks. Just out of curiosity why not simply merge/cherry-pick patches and instead re-apply them without original subject/author/description?

P.S. maybe you should put some name here?

tmux/COPYING

Line 6 in e4c4ceb

Copyright (c) <author>

@nicm
Copy link
Copy Markdown
Member

nicm commented Jul 10, 2023

OK, fair enough.

There are three reasons not to merge using Git:

  • I have to apply them to OpenBSD first and it uses CVS;
  • I want to apply the patch in order to review and test it and once I do that it is rare that there is a patch (or commit message) that doesn't need some minor tweak, so rather than bouncing back to the submitter to ask them to make some little change, it is much quicker if I just do it;
  • I don't like submitter information going into Git metadata rather than into the commit message.

COPYING is an example of the copyright header which appears at the top of each file, it doesn't need a name.

azat added a commit to azat/libevent that referenced this pull request Jul 10, 2023
signalfd may behave differently to sigaction/signal, so to avoid
breaking libevent users (like [1], [2]) disable it by default.

  [1]: tmux/tmux#3621
  [1]: tmux/tmux#3626

Also signalfd is not that perfect:
- you need to SIG_BLOCK the signal before
  - blocked signals are not reset on exec
  - blocked signals are allowed to coalesce - so in case of multiple
    signals sent you may get the signal only once (ok for most of the
    signals, but may be a problem for SIGCHLD, though you may call
    waitpid() in a loop or use pidfd)
- and also one implementation problem -
  sigprocmask is unspecified in a multithreaded process

Refs:
- https://lwn.net/Articles/415684/
- https://ldpreload.com/blog/signalfd-is-useless

Refs: libevent#1460
Refs: libevent#1342 (cc @dmantipov)
azat added a commit to azat/libevent that referenced this pull request Jul 10, 2023
signalfd may behave differently to sigaction/signal, so to avoid
breaking libevent users (like [1], [2]) disable it by default.

  [1]: tmux/tmux#3621
  [2]: tmux/tmux#3626

Also signalfd is not that perfect:
- you need to SIG_BLOCK the signal before
  - blocked signals are not reset on exec
  - blocked signals are allowed to coalesce - so in case of multiple
    signals sent you may get the signal only once (ok for most of the
    signals, but may be a problem for SIGCHLD, though you may call
    waitpid() in a loop or use pidfd)
- and also one implementation problem -
  sigprocmask is unspecified in a multithreaded process

Refs:
- https://lwn.net/Articles/415684/
- https://ldpreload.com/blog/signalfd-is-useless

Refs: libevent#1460
Refs: libevent#1342 (cc @dmantipov)
azat added a commit to azat/libevent that referenced this pull request Jul 13, 2023
signalfd may behave differently to sigaction/signal, so to avoid
breaking libevent users (like [1], [2]) disable it by default.

  [1]: tmux/tmux#3621
  [2]: tmux/tmux#3626

Also signalfd is not that perfect:
- you need to SIG_BLOCK the signal before
  - blocked signals are not reset on exec
  - blocked signals are allowed to coalesce - so in case of multiple
    signals sent you may get the signal only once (ok for most of the
    signals, but may be a problem for SIGCHLD, though you may call
    waitpid() in a loop or use pidfd)
- and also one implementation problem -
  sigprocmask is unspecified in a multithreaded process

Refs:
- https://lwn.net/Articles/415684/
- https://ldpreload.com/blog/signalfd-is-useless

Refs: libevent#1460
Refs: libevent#1342 (cc @dmantipov)
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 9, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

2 participants