Bug 887817 – Incorrect SIGINT handling (ebuild(1) doesn't terminate on ^C always)

• Product: Portage Development
• Status: RESOLVED FIXED

[Description Sam James 2022-12-22 01:32:47 UTC]

Reproducer:
```
/var/db/repos/gentoo/dev-vcs/rcs # while true ; do ebuild rcs-5.10.1.ebuild 
clean unpack ; done
```

Let it run for a second or two, then ^C^C^C^C^C. It won't abort immediately and often an exit can't be induced via CTRL-C (SIGINT) at all.

I was originally concerned that this was a regression with newer Bash as I hadn't noticed this behaviour before, but it seems not, after extensive discussion about a week ago in #gentoo-base.

The conclusion was mostly:
<tirnanog> programs that trap INT should be sure to kill themselves with INT after the trap handler has done whatever it set out to do, otherwise exactly that can happen. it's possible that ebuild(1) fails to do that.
<tirnanog> in summary, sys.exit(128 + SIGNUM) is tolerable but don't do it for QUIT/INT.
<@floppym> ebuild doesn't kill itself, but it does mess with the exit status.
<tirnanog> that's very, very wrong.
<tirnanog> it should make an exception for INT, in particular. I don't know how you write kill -INT $$ in python, but that, basically.
<tirnanog> either that or don't trap INT at all.
[...]
<@floppym> I haven't really made my mind up on whether we should just let KeyboardInterrupt go unhandled, or set the signal handler for SIGINT to SIG_DFL.
<@floppym> The former is cleaner from a python perspective, but will result in a backtrace when portage gets killed with Ctrl-C.

[Comment 1 dwfreed 2022-12-22 22:41:55 UTC]

<dwfreed> what about a combo approach? don't mess with signal handling normally, wrap main() in try/except KeyboardInterrupt, then change SIGINT to SIG_DFL and kill(0, SIGINT) ?
<dwfreed> you won't get the KeyboardInterrupt backtrace, but you'll still get exited-by-sigint status
<dwfreed> for emerge, which does actual work in a ctrl+c handler, just manually throw KeyboardInterrupt when that work is done, and keep the rest as above

[Comment 2 Mike Gilbert 2022-12-23 21:45:22 UTC]

After looking at it a bit, I don't see any obvious reason to handle KeyboardInterrupt instead of just setting SIG_DFL and letting the kernel kill the process immediately. Is there something I'm missing?

[Comment 3 Arsen Arsenović 2022-12-24 17:45:18 UTC]

It breaks context managers where they might be expected to work.  The option of wrapping the entry point in try: except KeyboardInterrupt: signal.signal(...); os.kill(...) seems no worse to me

[Comment 4 Mike Gilbert 2022-12-25 00:21:21 UTC]

(In reply to Arsen Arsenovic from comment #3)

It's somewhat annoying to implement since scripts like ebuild don't have a single "main" function we can wrap.

I guess I can do it if someone is aware of a context manager that actually needs to run before we terminate the process.

[Comment 5 Mike Gilbert 2022-12-27 04:59:11 UTC]

I went ahead and implemented the "catch KeyboardInterrupt" approach in a second PR. I think I like it better.

[Comment 6 Larry the Git Cow 2022-12-31 13:33:08 UTC]

The bug has been referenced in the following commit(s):

https://gitweb.gentoo.org/proj/portage.git/commit/?id=7141d7f0033bc7bf5bdf825271a0002657d4fb83

commit 7141d7f0033bc7bf5bdf825271a0002657d4fb83
Author:     Mike Gilbert <[e-mail]>
AuthorDate: 2022-12-27 03:57:56 +0000
Commit:     Sam James <[e-mail]>
CommitDate: 2022-12-31 13:29:24 +0000

    Rework signal handling in entry scripts
    
    Introduce a new exception SignalInterrupt which inherits from
    KeyboardInterrupt and adds a 'signum' member. When a signal is received,
    raise SignalInterrupt.
    
    At the end of the script, catch KeyboardInterrupt, and look for the
    signum member. Reset the signal handler to SIG_DFL, and re-raise the
    signal to kill the process.
    
    This ensures that the invoking shell sees that we got killed by a signal
    instead of calling exit.
    
    Bug: https://bugs.gentoo.org/887817
    Signed-off-by: Mike Gilbert <[e-mail]>
    Closes: https://github.com/gentoo/portage/pull/965
    Signed-off-by: Sam James <[e-mail]>

 bin/ebuild        |  781 +++++++--------
 bin/ebuild-ipc.py |  505 +++++-----
 bin/egencache     | 2473 +++++++++++++++++++++++-----------------------
 bin/emaint        |   86 +-
 bin/emerge        |   43 +-
 bin/portageq      | 2854 ++++++++++++++++++++++++++---------------------------
 6 files changed, 3381 insertions(+), 3361 deletions(-)

[Comment 7 Larry the Git Cow 2023-01-02 05:31:53 UTC]

The bug has been closed via the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=1d78c5d85797399b0dbd0fe885d04b7f50fc3a60

commit 1d78c5d85797399b0dbd0fe885d04b7f50fc3a60
Author:     Sam James <[e-mail]>
AuthorDate: 2023-01-02 05:31:32 +0000
Commit:     Sam James <[e-mail]>
CommitDate: 2023-01-02 05:31:42 +0000

    sys-apps/portage: add 3.0.43
    
    Closes: https://bugs.gentoo.org/888487
    Closes: https://bugs.gentoo.org/887817
    Closes: https://bugs.gentoo.org/885909
    Signed-off-by: Sam James <[e-mail]>

 sys-apps/portage/Manifest              |   1 +
 sys-apps/portage/portage-3.0.43.ebuild | 292 +++++++++++++++++++++++++++++++++
 2 files changed, 293 insertions(+)