[PATCH 1/2] Register SIGWINCH handler before TLS connect

By design, curses(3) must reinitialise its internal state after
resizing;  usually refresh(3) is called which in turn calls
doupdate(3) which is required for KEY_RESIZE to be picked up.
uiDraw() is simply an optimised refresh() but doupdate() is
all we need, so clarify/simplify accordingly.
---
 chat.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/chat.c b/chat.c
index 908bbce..a97f630 100644
--- a/chat.c
+++ b/chat.c
@@ -27,6 +27,7 @@
 
 #include <err.h>
 #include <errno.h>
+#include <curses.h>
 #include <fcntl.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -399,9 +400,7 @@ int main(int argc, char *argv[]) {
 		if (signals[SIGWINCH]) {
 			signals[SIGWINCH] = 0;
 			cursesWinch(SIGWINCH);
-			// XXX: For some reason, calling uiDraw() here is the only way to
-			// get uiRead() to properly receive KEY_RESIZE.
-			uiDraw();
+			doupdate();
 			uiRead();
 		}
 
-- 
2.32.0

> On Jun 19, 2021, at 23:46, Klemens Nanni <klemens@posteo.de> wrote:
> 
> By design, curses(3) must reinitialise its internal state after
> resizing;  usually refresh(3) is called which in turn calls
> doupdate(3) which is required for KEY_RESIZE to be picked up.
> uiDraw() is simply an optimised refresh() but doupdate() is
> all we need, so clarify/simplify accordingly.

I’d rather keep the call to uiDraw() even if it does extra work,
just to keep all the curses code contained to ui.c, but I’ll update
the comment with this explanation.

On Sun, Jun 20, 2021 at 03:46:30AM +0000, Klemens Nanni wrote:
> Otherwise resizing the terminal will end catgirl during ircConnect(),
> resulting in errors like "catgirl: tls_handshake: (null)".
> 
> Hoist it up to early UI init where logically fits the best;
> the other signals could be hoisted the same way but the practical
> difference would merely be the exit status of catgirl, so leave it.
> ---
>  chat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/chat.c b/chat.c
> index 479ec94..908bbce 100644
> --- a/chat.c
> +++ b/chat.c
> @@ -269,6 +269,7 @@ int main(int argc, char *argv[]) {
>  
>  	ircConfig(insecure, trust, cert, priv);
>  
> +	sig_t cursesWinch = signal(SIGWINCH, signalHandler);
>  	uiInitEarly();
>  	if (save) {
>  		uiLoad(save);

This fixed the obvious errors but wasn't quite right, either.

-- >8 --

Otherwise resizing the terminal will end catgirl until a handler is
registered, e.g. while in ircConnect():

	catgirl: tls_handshake: (null)

Hoist registration right after uiInitEarly() as earliest possible point
in main() since initscr(3) sets up various signals incl. SIGWINCH, i.e.
initialise `cursesWinch' afterwards to pick up curses(3)'s handler.
---
 chat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chat.c b/chat.c
index 479ec94..790038c 100644
--- a/chat.c
+++ b/chat.c
@@ -270,6 +270,7 @@ int main(int argc, char *argv[]) {
 	ircConfig(insecure, trust, cert, priv);
 
 	uiInitEarly();
+	sig_t cursesWinch = signal(SIGWINCH, signalHandler);
 	if (save) {
 		uiLoad(save);
 		atexit(exitSave);
@@ -324,7 +325,6 @@ int main(int argc, char *argv[]) {
 	signal(SIGALRM, signalHandler);
 	signal(SIGTERM, signalHandler);
 	signal(SIGCHLD, signalHandler);
-	sig_t cursesWinch = signal(SIGWINCH, signalHandler);
 
 	fcntl(irc, F_SETFD, FD_CLOEXEC);
 	bool pipes = !self.kiosk && !self.restricted;
-- 
2.32.0

> On Jun 20, 2021, at 14:50, Klemens Nanni <klemens@posteo.de> wrote:
> 
> Otherwise resizing the terminal will end catgirl until a handler is
> registered, e.g. while in ircConnect():
> 
> 	catgirl: tls_handshake: (null)

Sounds like the real problem there is that ircConnect() doesn’t
check the return value of tls_handshake(3) for
TLS_WANT_POLLIN/TLS_WANT_POLLOUT. I assume a signal handler is
causing EINTR somewhere in tls_handshake(3) and it needs to be
retried, but if so, this patch shouldn’t fix it either, unless it’s
ncurses installing its handler without SA_RESTART or something?

Does this fix that error? Kind of hard to reproduce.

https://git.causal.agency/catgirl/commit/?id=b3631a7e325ee98b73f6544ae7174d7c0e8a3025

On Sun, Jun 20, 2021 at 04:58:27PM -0400, june wrote:
> Does this fix that error? Kind of hard to reproduce.
> 
> https://git.causal.agency/catgirl/commit/?id=b3631a7e325ee98b73f6544ae7174d7c0e8a3025

That of course fixes tls_handshake(3) but still leaves out other places
like connect(2) in ircConnect();  it is harder to reproduce due to timing
but still happens every now and then when I start catgirl and immediately
keep resizing the window:


	$ catgirl -R -h irc.hackint.eu
	catgirl is GPLv3 fwee softwawe ^w^  code is avaiwable fwom https://git.causal.agency/catgirl
	Traveling...
	catgirl: irc.hackint.eu:6697: Can't assign requested address

So handling TLS_WANT_POLL* is nice but hoisting signal handler registration
still seems like a sensible thing to do in general.

On Mon, Jun 21, 2021 at 12:11:16AM +0200, Klemens Nanni wrote:
> On Sun, Jun 20, 2021 at 04:58:27PM -0400, june wrote:
> > Does this fix that error? Kind of hard to reproduce.
> > 
> > https://git.causal.agency/catgirl/commit/?id=b3631a7e325ee98b73f6544ae7174d7c0e8a3025
> 
> That of course fixes tls_handshake(3) but still leaves out other places
> like connect(2) in ircConnect();  it is harder to reproduce due to timing
> but still happens every now and then when I start catgirl and immediately
> keep resizing the window:
> 
> 
> 	$ catgirl -R -h irc.hackint.eu
> 	catgirl is GPLv3 fwee softwawe ^w^  code is avaiwable fwom https://git.causal.agency/catgirl
> 	Traveling...
> 	catgirl: irc.hackint.eu:6697: Can't assign requested address
> 
> So handling TLS_WANT_POLL* is nice but hoisting signal handler registration
> still seems like a sensible thing to do in general.


But you could also do this, I guess.  Downside is that this triggers
for all signals and not just SIGWINCH (not sure if that has any downsides).

-- >8 --

Subject: [PATCH] Handle EINTR from connect(2) gracefully

Resizing the window early on may return early due to SIGWINCH.
Continue asynchronously in that case instead of exiting.
---
 irc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/irc.c b/irc.c
index 3f0de3c..691cea6 100644
--- a/irc.c
+++ b/irc.c
@@ -27,6 +27,7 @@
 
 #include <assert.h>
 #include <err.h>
+#include <errno.h>
 #include <netdb.h>
 #include <netinet/in.h>
 #include <stdarg.h>
@@ -148,6 +149,7 @@ int ircConnect(const char *bindHost, const char *host, const char *port) {
 
 		error = connect(sock, ai->ai_addr, ai->ai_addrlen);
 		if (!error) break;
+		if (error && errno == EINTR) break;
 
 		close(sock);
 		sock = -1;
-- 
2.32.0

Applied both the connect(2) EINTR handling and the original (fixed)
patch.