[PATCH] errno management in curses.c

[Sebastian Gniazdowski <psprint3@xxxxxxxxxxxx>]

Hello,
comment in curses.c says:

     * The observed behavior, however, is different:  wgetch()
     consistently
     * returns ERR with EINTR when a signal is handled by the shell
     "trap"
     * command mechanism.  Further, it consistently returns ERR twice,
     the
     * second time without even attempting to repeat the interrupted
     read,
     * which has the side-effect of NOT updating errno.  A third call
     will
     * then begin reading again.
     *
     * Therefore, to properly implement signal trapping, we must (1)
     call
     * wgetch() in a loop as long as errno remains EINTR, and (2) clear
     * errno only before beginning the loop, not on every pass.

Logic:

1. The comment assumes EINTR being not reset by curses implies the read
will be "mock". It could assume that first read after EINTR will be mock
based on evidence, but it quietly assumes this for all subsequent reads.

- hidden assumption: errno is needed by curses to track its state after
interrupt (!).


2. The comment fits getch-like-calls without timeout. The loop:

    while ((ret = wget_wch(w->win, &wi)) == ERR) {
	if (errno != EINTR || errflag || retflag || breaks ||
	exit_pending)
	    break;
    }

will nicely detect EINTR after ERR return value, then go inside
wget_ch() again indefinitely long, then receive ~ERR and jump over errno
== EINTR. It also will skip the "mock" read that author probably
spotted.

-> conclusion: author designed the loop only for no-timeout-reads


3. Timeouts:

-> OS X, Linux man wget_wch: "In  no-delay  mode, if no input is
waiting, the value ERR is returned.", 
-> OS X, Linux man wtimeout: "The timeout and wtimeout routines set
blocking or non-blocking read for a given window. (...) If delay is
positive, then read blocks for delay milliseconds, and returns ERR if
there is still no input."

So, clash happens – errno is not reset (see below), ERR is returned
after timeout, and loop hangs.


4. Ncurses can reset or not errno. We should now note: it is the
possibility-of-no-reset that matters. Not possible-reset. If there is
any curses library that doesn't reset errno, then the loop is wrong.
Proof for ncurses 5.4 (browse for "errno"):

https://github.com/psprint/zsh-tools-private/blob/master/data/lib_getch.c


5. Library-not-resetting-errno is the standard:
https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=6619179


The patch resets errno after ERR/EINTR. Removed also most of the long
comment. It was hard to find solution because of that comment, it was
making impression of some inherent drastic problems with diverse curses
implementations, while there are none, the Linux, FreeBSD documentation
just says about getch being either interrupted or not, THAT'S ALL. No
internet page exists that mentions any problems here.

-- 
  Sebastian Gniazdowski
  psprint3@xxxxxxxxxxxx

diff --git a/Src/Modules/curses.c b/Src/Modules/curses.c
index 63c6748..d9c19bd 100644
--- a/Src/Modules/curses.c
+++ b/Src/Modules/curses.c
@@ -1082,15 +1082,7 @@ zccmd_input(const char *nam, char **args)
 #endif
 
     /*
-     * Some documentation for wgetch() says:
-
-       The behavior of getch and friends in the presence of  handled  signals
-       is  unspecified  in the SVr4 and XSI Curses documentation.  Under his-
-       torical curses implementations, it varied  depending  on  whether  the
-       operating system's implementation of handled signal receipt interrupts
-       a read(2) call in progress or not, and also (in some  implementations)
-       depending  on  whether  an input timeout or non-blocking mode has been
-       set.
+     * Linux, OS X, FreeBSD documentation for wgetch() mentions:
 
        Programmers concerned about portability should be prepared for  either
        of  two cases: (a) signal receipt does not interrupt getch; (b) signal
@@ -1098,21 +1090,16 @@ zccmd_input(const char *nam, char **args)
        EINTR.  Under the ncurses implementation, handled signals never inter-
        rupt getch.
 
-     * The observed behavior, however, is different:  wgetch() consistently
-     * returns ERR with EINTR when a signal is handled by the shell "trap"
-     * command mechanism.  Further, it consistently returns ERR twice, the
-     * second time without even attempting to repeat the interrupted read,
-     * which has the side-effect of NOT updating errno.  A third call will
-     * then begin reading again.
-     *
-     * Therefore, to properly implement signal trapping, we must (1) call
-     * wgetch() in a loop as long as errno remains EINTR, and (2) clear
-     * errno only before beginning the loop, not on every pass.
+     * Some observed behavior: wgetch() returns ERR with EINTR when a signal is
+     * handled by the shell "trap" command mechanism. Observed that it returns
+     * ERR twice, the second time without even attempting to repeat the
+     * interrupted read. Third call will then begin reading again.
      *
-     * There remains a potential bug here in that, if the caller has set
-     * a timeout for the read [see zccmd_timeout()] the countdown is very
-     * likely restarted on every call to wgetch(), so an interrupted call
-     * might wait much longer than desired.
+     * Because of widespread of previous implementation that called wget*ch
+     * possibly indefinitely many times after ERR/EINTR, and because of the
+     * above observation, wget_wch call is repeated after each ERR/EINTR, but
+     * errno is being reset (it wasn't) and the loop to all means should break.
+     * Problem: the timeout may be waited twice.
      */
     errno = 0;
 
@@ -1120,6 +1107,7 @@ zccmd_input(const char *nam, char **args)
     while ((ret = wget_wch(w->win, &wi)) == ERR) {
 	if (errno != EINTR || errflag || retflag || breaks || exit_pending)
 	    break;
+        errno = 0;
     }
     switch (ret) {
     case OK:
@@ -1146,6 +1134,7 @@ zccmd_input(const char *nam, char **args)
     while ((ci = wgetch(w->win)) == ERR) {
 	if (errno != EINTR || errflag || retflag || breaks || exit_pending)
 	    return 1;
+        errno = 0;
     }
     if (ci >= 256) {
 	keypadnum = ci;