Cataclysm-DDA icon indicating copy to clipboard operation
Cataclysm-DDA copied to clipboard

main: properly handle SIGINT

Open andrei8l opened this issue 1 year ago • 12 comments

Summary

None

Purpose of change

  • We're handling SIGINT (Ctr+C) by killing ourselves
  • Killing ourselves doesn't immediately terminate execution
  • Sometimes the OS doesn't terminate us in time and execution proceeds as if SIGINT was cancelled and the game tries to refresh display, leading to a segfault since we've already manually destroyed some game objects.
  • Additionally, the sig handler does not comply with MSC54-CPP, so we've been relying on UB this entire time
  • Fixes: #8417

  • Fixes: #77011

  • Closes: #76173

  • Closes: #77050 these both do similar things in that they try to move execution to a less crashy path before the OS takes over, but don't address the core issue.

Describe the solution

Split SIGINT handling to a separate function Move all non-SIG30-C-compliant code out of the sig handler Add a new quit reason (QUIT_EXIT) and pass it up to the main loop as an exception, then exit gracefully.

Describe alternatives you've considered

Give up on pedantry and just revert #67893 since the old code worked fine and for all edge cases despite being wrong.

Instead of passing an exception, I could pepper QUIT_EXIT checks all over the place.

Testing

Send SIGINT or other signals, or try to close the window while playing the game in various states: opening menu, loading files, finalizing data, verifying data, loading save file, waiting for input, doing an activity, and in various UIs. The game should prompt and quit correctly with no crashes.

Additional context

It can take a long time for the prompt to appear during some init stages. More QUIT_EXIT checks can be peppered around the code if needed.

UIs that don't use a QUIT keybinding won't react to SIGINT.

There might be other edge cases. Doing it correctly without an unified input loop is hard work.

andrei8l avatar Aug 27 '24 18:08 andrei8l

This fixes SIGINT, but the linked issue only talks about SIGTERM. I need to double check that

andrei8l avatar Aug 27 '24 18:08 andrei8l

Could close the 10 years old #8417 too ?

alef avatar Aug 27 '24 19:08 alef

Does this fix the root cause of the heap overflow, causing corrupted crash path locations?

NetSysFire avatar Aug 27 '24 20:08 NetSysFire

Could close the 10 years old #8417 too ?

It does now, thanks.

Does this fix the root cause of the heap overflow, causing corrupted crash path locations?

This PR turned out to fix an unrelated crash.

andrei8l avatar Aug 27 '24 20:08 andrei8l

to be honest I dont think my PR should be invalidated until theres a proper fix for the mystery heap overflow. I already did everything I can to debug this.

NetSysFire avatar Aug 27 '24 21:08 NetSysFire

to be honest I dont think my PR should be invalidated until theres a proper fix for the mystery heap overflow. I already did everything I can to debug this.

There is no mystery. The crash log shows the problem fairly clearly. There are just multiple, unrelated instances of the same issue. In any case, your PR just hides the issue under the rug as I've tried to explain on discord as well.

andrei8l avatar Aug 27 '24 21:08 andrei8l

So the other PR does fix the underlying issue leading to the heap overflow?

NetSysFire avatar Aug 27 '24 23:08 NetSysFire

It seems like the quit dialog gets stuck if you send another sigint while it's open.

Otherwise from some quick testing this seems to work as expected :+1:

NB: only tested this on tiles

harakka avatar Oct 20 '24 07:10 harakka

It seems like the quit dialog gets stuck if you send another sigint while it's open.

Thanks for catching that. It should be fixed now.

andrei8l avatar Oct 20 '24 07:10 andrei8l

I can still reproduce it, eg. on tiles, right on the main menu before loading the game, after two sigints nothing can be selected in the dialog.

Also I noticed on the main menu, when selecting No, the game still closes.

harakka avatar Oct 20 '24 08:10 harakka

I can still reproduce it, eg. on tiles, right on the main menu before loading the game, after two sigints nothing can be selected in the dialog.

Also I noticed on the main menu, when selecting No, the game still closes.

Fixed. I'll leave this in draft for a bit, in case you find other things. Thanks a lot for testing!

andrei8l avatar Oct 20 '24 08:10 andrei8l

Tested both tiles and curses, seems to work fine now :+1:

harakka avatar Oct 20 '24 09:10 harakka

Thanks for merging.

andrei8l avatar Oct 21 '24 14:10 andrei8l