PDCurses' endwin implementation is not signal-safe

[GitMensch]

When an application is in curses mode either the application or curses should take care to "reset" the screen when it exits.

One of the things that a curses application may does is to register an error handler (commonly a signal handler) which calles endwin(). It should not call delwin() as the necessary free() calls in there are problematic and "the application is going to exit afterwards in any case". Also: if the OS (or application) creates a coredump afterwards delwin() is likely to clean some traces which may be useful to debug some issues, and also for a signal handler "faster is better".

This often works fine - but the chances with PDCurses (@Bill-Gray also applies to PDCursesMod) are less good.

One of the reasons is the use of PDC_LOG macro which itself calls some unsafe functions, for many ports one of the reasons is also the check of PDC_RESTORE_SCREEN in PDC_scr_close() as this uses getenv() which should not be used in a signal handler, see the manpage for signal-safety.

Note: this is not a theoretic issue - I've recently debugged a bunch of memory issues in an application and if its signal handler is called because of memory issues in free or malloc, then the call to endwin() in there currently is not unlikely to create a lock issue or re-raises a SIGSEGV (or in case of glibc checks a SIGABRT) so the rest of the signal handler is not executed. This potentially prevents generation of coredumps, too :-(

My suggestions:

• only read PDC_RESTORE_SCREEN in initscr() and place the result in a static var, possibly add a function to toggle it later in the application (or implicit re-read it at other places like refresh() or getch())
• drop the actual logging from endwin() and called functions, or only do the logging depending on static variables (see suggestion for PDC_RESTORE_SCREEN)
• declare signal-safety for PDCurses' endwin() (and drop an implementation use about the functions to (not) use in PDC_scr_close()

Related (but does not need to be adjusted together) #133.

[Bill-Gray]

(Following applies to PDCurses and PDCursesMod).

Your first bullet point doesn't apply to WinCon, because it already only reads PDC_RESTORE_SCREEN at startup. It does apply to both OS/2 and DOS. In both cases, it looks to me as if there is a static saved_screen variable; it's allocated to a non-NULL value in pdc_scr_open() if the PDC_RESTORE_SCREEN environment variable is set, and left to NULL if it isn't. So we don't need to store the getenv() result; just looking at saved_screen tells us what we need to know.

I don't see any provision made for PDC_RESTORE_SCREEN changing in a sensible way. If it was set at startup, and saved_screen is non-NULL, then saved_screen still ought to be deallocated in PDC_scr_close(), regardless of any environment variables.

I think the only thing we need do is modify, in PDC_scr_close(),

if( saved_screen && getenv("PDC_RESTORE_SCREEN"))

to read

if( saved_screen)

This both ensures that saved_screen is properly freed, even if the environment variable got reset, and gets rid of a signal-unsafe call.

Making endwin() completely signal-safe doesn't look easy. Best I can come up with is to use this new function as your signal handler instead :

int signal_safe_endwin( void)
{
   SP->we_are_in_a_signal_handler = TRUE;
   return endwin( );
}

...basically, an endwin() for use only within signal handlers. We'd add a new boolean element of the SP struct, SP->we_are_in_a_signal_handler. Various bits of code would then preface any signal-unsafe calls with if(!SP->we_are_in_a_signal_handler).

The inability to call signal-unsafe functions would mean that not all memory would be freed and might have some other consequences not immediately apparent to me.

Alternatively, is there a way, in either DOS or OS/2, for endwin() (or any function) to know that it's been called within a signal handler?

[GitMensch]

I don't see any provision made for PDC_RESTORE_SCREEN changing in a sensible way. If it was set at startup, and saved_screen is non-NULL, then saved_screen still ought to be deallocated in PDC_scr_close(), regardless of any environment variables.

That shouldn't be done for multitude of reasons, one is that a refresh() should get the last state back and you'd want to be able to endwin() again and get the saved screen again.

The inability to call signal-unsafe functions would mean that not all memory would be freed and might have some other consequences not immediately apparent to me.

As noted above and also in the unix specification for endwin():

The endwin() function does not free storage associated with a screen, so delscreen() should be called after endwin() if a particular screen is no longer needed.

If there are other reasons then yes - a signal-safe endwin would be useful.

Changing the logging state during the run is a different thing and definitely useful - maybe this can be done with a function call instead of the environment (so environment is only read during initscr()?

[Bill-Gray]

That shouldn't be done for multitude of reasons, one is that a refresh() should get the last state back and you'd want to be able to endwin() again and get the saved screen again.

Ugh. You are correct. At present, in ncurses, DOS, VT, and OS/2, if you call endwin(), the screen would be restored and Curses is effectively "off". But you can then call refresh() to effectively restore PDCurses. (Which I do in some of my programs; with some, I'll have a hotkey that does so to let me see what was on the screen before the program started.)

Under VT and ncurses, the original screen will still be saved, and when you call endwin() a second or third time, it'll be restored. In DOS or OS/2, it'll only get restored the first time.

The bit about not calling delscreen(), I'm fairly sure, refers to the curses function of that name (and to the SCREEN structure). Various demos ensure that all memory is freed by calling, after endwin(),

#ifdef PDCURSES
   delscreen( SP);
#endif 

The saved_screen variables in the DOS and OS/2 ports are not Curses screen structures (or even the same as each other). Freeing them would be Just Fine, except for this bit about only being able to restore the screen once.

Changing the logging state during the run is a different thing and definitely useful - maybe this can be done with a function call instead of the environment (so environment is only read during initscr()?
We have traceon(), traceoff(), trace(), curses_trace(). I've sometimes turned tracing on briefly for one key bit of code I'm trying to figure out.

At present, if you turn any logging on and then shut it off, we do an fclose(), which is signal-unsafe. We could, instead, simply stop logging until tracing is turned back on.

[Bill-Gray]

@GitMensch - this should be fixed in the PDCursesMod fork now. Give it a try and see what you think.

Solution was in two (fairly easy) parts, which could also apply to PDCurses. First, those memory allocations in the DOS and OS/2 platforms are not freed in PDC_scr_close(). That led to a need for a new PDC_free_memory_allocations() function, which frees both the non-platform-dependent allocations (clipboard, debugging file pointer, delscreen(SP)) and the platform-dependent allocations. At least at present, that just means the blocks allocated in DOS and OS/2; all other platforms have nothing to free. (Or, in SDLn and X11, lots to free, but no way to do so.)

Second, we set an SP->in_endwin Boolean to TRUE; while it's thus, logging is disabled.

As noted above, the first fix also fixes a bug on DOS and OS/2 : if you called endwin() and refresh(), the saved screen would be freed, meaning that if you did it a second time, there would be no saved screen to restore. Now, we keep that screen until you call PDC_free_memory_allocations(), basically saying "I'm totally done with PDCurses".

[GitMensch]

Obviously PCursesMod removed PDC_free_memory_allocations (which is not part of PDCurses); but still there are changes to "handle memory correct".

@Bill-Gray: could you please summarize the necessary changes / link the appropriate commits so that this may be handled correctly here, too?

[Bill-Gray]

@GitMensch - can't say I wound up doing much for this (it didn't take many changes). Commit Bill-Gray/PDCursesMod@18ff2cb00c040a3 added a bit of logic to say that if we're in endwin(), debugging is turned off. In DOS and OS/2, we only check the PDC_RESTORE_SCREEN environment variable at startup; checking for it later wasn't a good idea anyway, even had it been signal-safe (as noted above, it was buggy if you called endwin(), refresh(), then did so again, on both of those platforms.

That was basically all I did. Details would vary in PDCurses, but the fixes would be the same.