telnet: various fixes.

This got a bit out of hand. All I wanted to fix was the CR conversion to
get this part of https://tools.ietf.org/html/rfc854 right:

  Therefore, the sequence "CR LF" must be treated as a single "new
  line" character and used whenever their combined action is
  intended; the sequence "CR NUL" must be used where a carriage
  return alone is actually desired; and the CR character must be
  avoided in other contexts.  This rule gives assurance to systems
  which must decide whether to perform a "new line" function or a
  multiple-backspace that the TELNET stream contains a character
  following a CR that will allow a rational decision.

But to understand the code well enough to do that, and to fix it so that
it works when IAC or CR sequences are split across multiple reads, I
ended up rewriting a lot:

* Add punctuation in help.

* Remove duplicated #include.

* Remove some unnecessary globals, enlarge the global buffers, and keep
  state for correct IAC sequence processing across reads.

* Reduce code duplication and rewrite bits that made no sense.

* Handle entering/exiting raw mode more uniformly.

* Fix the prompt (the character count was wrong).

* Allow ^]^D (like BSD telnet) as well as ^]e to exit, and look less
  like we crashed when doing so.

* Simplify the IAC sequence handling, but more importantly work
  correctly when a sequence is split across multiple reads.

* Use more of the existing "x" functions from lib. (And remove code that
  was duplicating what the "x" functions they'd just called had already
  done.)

* Show "Connected to".

* Better signal handling.

I'm still not happy with TELOPT_ECHO and TELOPT_SGA in handle_wwdd(),
but don't (yet) understand them well enough to simplify them further. On
the bright side, I think TELOPT_NAWS is a lot clearer now.

It also occurs to me now I'm looking at the diff that although the IAC
output code is now better than it was, it probably still isn't pulling
its weight and might better be replaced by printf().

...but this patch has already gotten way out of hand!

1 files changed, 187 insertions, 229 deletions

diff --git a/toys/pending/telnet.c b/toys/pending/telnet.c
index b4d5c72f..a7ea91e2 100644
--- a/toys/pending/telnet.c
+++ b/toys/pending/telnet.c
@@ -14,329 +14,287 @@ config TELNET
   help
     usage: telnet HOST [PORT]
 
-    Connect to telnet server
+    Connect to telnet server.
 */
 
 #define FOR_telnet
 #include "toys.h"
 #include <arpa/telnet.h>
-#include <netinet/in.h>
 
 GLOBALS(
-  int port;
-  int sfd;
-  char buff[128];
-  int pbuff;
-  char iac[256];
-  int piac;
-  char *ttype;
-  struct termios def_term;
+  int sock;
+  char buf[2048]; // Half sizeof(toybuf) allows a buffer full of IACs.
+  char iac[128];
+  int iac_len;
+  struct termios old_term;
   struct termios raw_term;
-  uint8_t term_ok;
-  uint8_t term_mode;
-  uint8_t flags;
-  unsigned win_width;
-  unsigned win_height;
+  uint8_t mode;
+  int echo, sga;
+  int state, request;
 )
 
-#define DATABUFSIZE 128
-#define IACBUFSIZE  256
+#define NORMAL 0
+#define SAW_IAC 1
+#define SAW_WWDD 2
+#define SAW_SB 3
+#define SAW_SB_TTYPE 4
+#define WANT_IAC 5
+#define WANT_SE 6
+#define SAW_CR 10
+
 #define CM_TRY      0
 #define CM_ON       1
 #define CM_OFF      2
-#define UF_ECHO     0x01
-#define UF_SGA      0x02
 
-// sets terminal mode: LINE or CHARACTER based om internal stat.
-static char const es[] = "\r\nEscape character is ";
-static void set_mode(void)
+static void raw(int raw)
 {
-  if (TT.flags & UF_ECHO) {
-    if (TT.term_mode == CM_TRY) {
-      TT.term_mode = CM_ON;
-      printf("\r\nEntering character mode%s'^]'.\r\n", es);
-      if (TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.raw_term);
-    }
-  } else {
-    if (TT.term_mode != CM_OFF) {
-      TT.term_mode = CM_OFF;
-      printf("\r\nEntering line mode%s'^C'.\r\n", es);
-      if (TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.def_term);
-    }
-  }
+  tcsetattr(0, TCSADRAIN, raw ? &TT.raw_term : &TT.old_term);
 }
 
-// flushes all data in IAC buff to server.
 static void flush_iac(void)
 {
-  int wlen = write(TT.sfd, TT.iac, TT.piac);
-
-  if(wlen <= 0) error_msg("IAC : send failed.");
-  TT.piac = 0;
+  xwrite(TT.sock, TT.iac, TT.iac_len);
+  TT.iac_len = 0;
 }
 
-// puts DATA in iac buff of length LEN and updates iac buff pointer.
-static void put_iac(int len, ...)
+static void iac(int n, ...)
 {
   va_list va; 
 
-  if(TT.piac + len >= IACBUFSIZE) flush_iac();
-  va_start(va, len);
-  for(;len > 0; TT.iac[TT.piac++] = (uint8_t)va_arg(va, int), len--);
+  if (TT.iac_len + n >= sizeof(TT.iac)) flush_iac();
+  va_start(va, n);
+  while (n--) TT.iac[TT.iac_len++] = va_arg(va, int);
   va_end(va);
 }
 
-// puts string STR in iac buff and updates iac buff pointer.
-static void str_iac(char *str)
+static void iacstr(char *str)
 {
-  int len = strlen(str);
+  if (TT.iac_len) flush_iac();
+  xwrite(TT.sock, str, strlen(str));
+  TT.iac_len = 0;
+}
 
-  if(TT.piac + len + 1 >= IACBUFSIZE) flush_iac();
-  strcpy(&TT.iac[TT.piac], str);
-  TT.piac += len+1;
+static void slc(int line)
+{
+  TT.mode = line ? CM_OFF : CM_ON;
+  xprintf("Entering %s mode\r\nEscape character is '^%c'.\r\n",
+      line ? "line" : "character", line ? 'C' : ']');
+  raw(!line);
+}
+
+static void set_mode(void)
+{
+  if (TT.echo) {
+    if (TT.mode == CM_TRY) slc(0);
+  } else if (TT.mode != CM_OFF) slc(1);
 }
 
 static void handle_esc(void)
 {
   char input;
 
-  if(toys.signal && TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.raw_term);
-  xwrite(1,"\r\nConsole escape. Commands are:\r\n\n"
+  if (toys.signal) raw(1);
+
+  // This matches busybox telnet, not BSD telnet.
+  xputsn("\r\n"
+      "Console escape. Commands are:\r\n"
+      "\r\n"
       " l  go to line mode\r\n"
       " c  go to character mode\r\n"
       " z  suspend telnet\r\n"
-      " e  exit telnet\r\n", 114);
-
-  if (read(0, &input, 1) <= 0) {
-    if(TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.def_term);
-    exit(0);
+      " e  exit telnet\r\n"
+      "\r\n"
+      "telnet> ");
+  // In particular, the boxes only read a single character, not a line.
+  if (read(0, &input, 1) <= 0 || input == 4 || input == 'e') {
+    xprintf("Connection closed.\r\n");
+    xexit();
   }
 
-  switch (input) {
-  case 'l':
+  if (input == 'l') {
     if (!toys.signal) {
-      TT.term_mode = CM_TRY;
-      TT.flags &= ~(UF_ECHO | UF_SGA);
+      TT.mode = CM_TRY;
+      TT.echo = TT.sga = 0;
       set_mode();
-      put_iac(6, IAC,DONT,TELOPT_ECHO,IAC,DONT, TELOPT_SGA);
+      iac(6, IAC, DONT, TELOPT_ECHO, IAC, DONT, TELOPT_SGA);
       flush_iac();
       goto ret;
     }
-    break;
-  case 'c':
+  } else if (input == 'c') {
     if (toys.signal) {
-      TT.term_mode = CM_TRY;
-      TT.flags |= (UF_ECHO | UF_SGA);
+      TT.mode = CM_TRY;
+      TT.echo = TT.sga = 1;
       set_mode();
-      put_iac(6, IAC,DO,TELOPT_ECHO,IAC,DO,TELOPT_SGA);
+      iac(6, IAC, DO, TELOPT_ECHO, IAC, DO, TELOPT_SGA);
       flush_iac();
       goto ret;
     }
-    break;
-  case 'z':
-    if(TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.def_term);
+  } else if (input == 'z') {
+    raw(0);
     kill(0, SIGTSTP);
-    if(TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.raw_term);
-    break;
-  case 'e':
-    if(TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.def_term);
-    exit(0);
-  default: break;
+    raw(1);
   }
 
-  xwrite(1, "continuing...\r\n", 15);
-  if (toys.signal && TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.def_term);
+  xprintf("telnet %s %s\r\n", toys.optargs[0], toys.optargs[1] ?: "23");
+  if (toys.signal) raw(0);
 
 ret:
   toys.signal = 0;
 }
 
-/*
- * handles telnet SUB NEGOTIATIONS
- * only terminal type is supported.
- */
-static void handle_negotiations(void)
+// Handle WILL WONT DO DONT requests from the server.
+static void handle_wwdd(char opt)
 {
-  char opt = TT.buff[TT.pbuff++];
-
-  switch(opt) {
-  case TELOPT_TTYPE:
-    opt =  TT.buff[TT.pbuff++];
-    if(opt == TELQUAL_SEND) {
-      put_iac(4, IAC,SB,TELOPT_TTYPE,TELQUAL_IS);
-      str_iac(TT.ttype);
-      put_iac(2, IAC,SE);
-    }
-    break;
-  default: break;
-  }
-}
-
-/*
- * handles server's DO DONT WILL WONT requests.
- * supports ECHO, SGA, TTYPE, NAWS
- */
-static void handle_ddww(char ddww)
-{
-  char opt = TT.buff[TT.pbuff++];
-
-  switch (opt) {
-  case TELOPT_ECHO: /* ECHO */
-    if (ddww == DO) put_iac(3, IAC,WONT,TELOPT_ECHO);
-    if(ddww == DONT) break;
-    if (TT.flags & UF_ECHO) {
-        if (ddww == WILL) return;
-      } else if (ddww == WONT) return;
-    if (TT.term_mode != CM_OFF) TT.flags ^= UF_ECHO;
-    (TT.flags & UF_ECHO)? put_iac(3, IAC,DO,TELOPT_ECHO) : 
-      put_iac(3, IAC,DONT,TELOPT_ECHO);
+  if (opt == TELOPT_ECHO) {
+    if (TT.request == DO) iac(3, IAC, WONT, TELOPT_ECHO);
+    if (TT.request == DONT) return;
+    if (TT.echo) {
+        if (TT.request == WILL) return;
+    } else if (TT.request == WONT) return;
+    if (TT.mode != CM_OFF) TT.echo ^= 1;
+    iac(3, IAC, TT.echo ? DO : DONT, TELOPT_ECHO);
     set_mode();
-    printf("\r\n");
-    break;
-
-  case TELOPT_SGA: /* Supress GO Ahead */
-    if (TT.flags & UF_SGA){ if (ddww == WILL) return;
-    } else if (ddww == WONT) return;
-
-    TT.flags ^= UF_SGA;
-    (TT.flags & UF_SGA)? put_iac(3, IAC,DO,TELOPT_SGA) :
-      put_iac(3, IAC,DONT,TELOPT_SGA);
-    break;
-
-  case TELOPT_TTYPE: /* Terminal Type */
-    (TT.ttype)? put_iac(3, IAC,WILL,TELOPT_TTYPE):
-      put_iac(3, IAC,WONT,TELOPT_TTYPE);
-    break;
-
-  case TELOPT_NAWS: /* Window Size */
-    put_iac(3, IAC,WILL,TELOPT_NAWS);
-    put_iac(9, IAC,SB,TELOPT_NAWS,(TT.win_width >> 8) & 0xff,
-        TT.win_width & 0xff,(TT.win_height >> 8) & 0xff,
-        TT.win_height & 0xff,IAC,SE);
-    break;
-
-  default: /* Default behaviour is to say NO */
-    if(ddww == WILL) put_iac(3, IAC,DONT,opt);
-    if(ddww == DO) put_iac(3, IAC,WONT,opt);
-    break;
+  } else if (opt == TELOPT_SGA) { // Suppress Go Ahead
+    if (TT.sga) {
+      if (TT.request == WILL) return;
+    } else if (TT.request == WONT) return;
+    TT.sga ^= 1;
+    iac(3, IAC, TT.sga ? DO : DONT, TELOPT_SGA);
+  } else if (opt == TELOPT_TTYPE) { // Terminal TYPE
+    iac(3, IAC, WILL, TELOPT_TTYPE);
+  } else if (opt == TELOPT_NAWS) { // Negotiate About Window Size
+    unsigned cols = 80, rows = 24;
+
+    terminal_size(&cols, &rows);
+    iac(3, IAC, WILL, TELOPT_NAWS);
+    iac(7, IAC, SB, TELOPT_NAWS, cols>>8, cols, rows>>8, rows);
+    iac(2, IAC, SE);
+  } else {
+    // Say "no" to anything we don't understand.
+    iac(3, IAC, (TT.request == WILL) ? DONT : WONT, opt);
   }
 }
 
-/*
- * parses data which is read from server of length LEN.
- * and passes it to console.
- */
-static int read_server(int len)
+static void handle_server_output(int n)
 {
+  char *p = TT.buf, *end = TT.buf + n, ch;
   int i = 0;
-  char curr;
-  TT.pbuff = 0;
-
-  do {
-    curr = TT.buff[TT.pbuff++];
-    if (curr == IAC) {
-      curr = TT.buff[TT.pbuff++];
-      switch (curr) {
-      case DO:    /* FALLTHROUGH */
-      case DONT:    /* FALLTHROUGH */
-      case WILL:    /* FALLTHROUGH */
-      case WONT:
-        handle_ddww(curr);
-        break;
-      case SB:
-        handle_negotiations();
-        break;
-      case SE:
-        break;
-      default: break;
+
+  // Possibilities:
+  //
+  // 1. Regular character
+  // 2. IAC [WILL|WONT|DO|DONT] option
+  // 3. IAC SB option arg... IAC SE
+  //
+  // The only subnegotiation we support is IAC SB TTYPE SEND IAC SE, so we just
+  // hard-code that into our state machine rather than having a more general
+  // "collect the subnegotation into a buffer and handle it after we've seen
+  // the IAC SE at the end". It's 2021, so we're unlikely to need more.
+
+  while (p < end) {
+    ch = *p++;
+    if (TT.state == SAW_IAC) {
+      if (ch >= WILL && ch <= DONT) {
+        TT.state = SAW_WWDD;
+        TT.request = ch;
+      } else if (ch == SB) {
+        TT.state = SAW_SB;
+      } else {
+        TT.state = NORMAL;
       }
-    } else {
-      toybuf[i++] = curr;
-      if (curr == '\r') { curr = TT.buff[TT.pbuff++];
-        if (curr != '\0') TT.pbuff--;
+    } else if (TT.state == SAW_WWDD) {
+      handle_wwdd(ch);
+      TT.state = NORMAL;
+    } else if (TT.state == SAW_SB) {
+      if (ch == TELOPT_TTYPE) TT.state = SAW_SB_TTYPE;
+      else TT.state = WANT_IAC;
+    } else if (TT.state == SAW_SB_TTYPE) {
+      if (ch == TELQUAL_SEND) {
+        iac(4, IAC, SB, TELOPT_TTYPE, TELQUAL_IS);
+        iacstr(getenv("TERM") ?: "NVT");
+        iac(2, IAC, SE);
       }
+      TT.state = WANT_IAC;
+    } else if (TT.state == WANT_IAC) {
+      if (ch == IAC) TT.state = WANT_SE;
+    } else if (TT.state == WANT_SE) {
+      if (ch == SE) TT.state = NORMAL;
+    } else if (ch == IAC) {
+      TT.state = SAW_IAC;
+    } else {
+      if (TT.state == SAW_CR && ch == '\0') {
+        // CR NUL -> CR
+      } else toybuf[i++] = ch;
+      if (ch == '\r') TT.state = SAW_CR;
+      TT.state = NORMAL;
     }
-  } while (TT.pbuff < len);
-
+  }
   if (i) xwrite(0, toybuf, i);
-  return 0;
 }
 
-/*
- * parses data which is read from console of length LEN
- * and passes it to server.
- */
-static void write_server(int len)
+static void handle_user_input(int n)
 {
-  char *c = (char*)TT.buff;
+  char *p = TT.buf, ch;
   int i = 0;
 
-  for (; len > 0; len--, c++) {
-    if (*c == 0x1d) {
+  while (n--) {
+    ch = *p++;
+    if (ch == 0x1d) {
       handle_esc();
       return;
     }
-    toybuf[i++] = *c;
-    if (*c == IAC) toybuf[i++] = *c; /* IAC -> IAC IAC */
-    else if (*c == '\r') toybuf[i++] = '\0'; /* CR -> CR NUL */
+    toybuf[i++] = ch;
+    if (ch == IAC) toybuf[i++] = IAC; // IAC -> IAC IAC
+    else if (ch == '\r') toybuf[i++] = '\n'; // CR -> CR LF
+    else if (ch == '\n') { // LF -> CR LF
+      toybuf[i-1] = '\r';
+      toybuf[i++] = '\n';
+    }
   }
-  if(i) xwrite(TT.sfd, toybuf, i);
+  if (i) xwrite(TT.sock, toybuf, i);
+}
+
+static void reset_terminal(void)
+{
+  raw(0);
 }
 
 void telnet_main(void)
 {
-  char *port = "23";
-  int set = 1, len;
   struct pollfd pfds[2];
+  int n = 1;
 
-  TT.win_width = 80; //columns
-  TT.win_height = 24; //rows
+  tcgetattr(0, &TT.old_term);
+  TT.raw_term = TT.old_term;
+  cfmakeraw(&TT.raw_term);
 
-  if (toys.optc == 2) port = toys.optargs[1];
+  TT.sock = xconnectany(xgetaddrinfo(*toys.optargs, toys.optargs[1] ?: "23", 0,
+      SOCK_STREAM, IPPROTO_TCP, 0));
+  xsetsockopt(TT.sock, SOL_SOCKET, SO_KEEPALIVE, &n, sizeof(n));
 
-  TT.ttype = getenv("TERM");
-  if(!TT.ttype) TT.ttype = "";
-  if(strlen(TT.ttype) > IACBUFSIZE-1) TT.ttype[IACBUFSIZE - 1] = '\0';
+  xprintf("Connected to %s.\r\n", *toys.optargs);
 
-  if (!tcgetattr(0, &TT.def_term)) {
-    TT.term_ok = 1;
-    TT.raw_term = TT.def_term;
-    cfmakeraw(&TT.raw_term);
-  }
-  terminal_size(&TT.win_width, &TT.win_height);
-
-  TT.sfd = xconnectany(xgetaddrinfo(*toys.optargs, port, 0, SOCK_STREAM,
-    IPPROTO_TCP, 0));
-  setsockopt(TT.sfd, SOL_SOCKET, SO_REUSEADDR, &set, sizeof(set));
-  setsockopt(TT.sfd, SOL_SOCKET, SO_KEEPALIVE, &set, sizeof(set));
+  sigatexit(reset_terminal);
+  signal(SIGINT, generic_signal);
 
   pfds[0].fd = 0;
   pfds[0].events = POLLIN;
-  pfds[1].fd = TT.sfd;
+  pfds[1].fd = TT.sock;
   pfds[1].events = POLLIN;
-
-  signal(SIGINT, generic_signal);
-  while(1) {
-    if(TT.piac) flush_iac();
-    if(poll(pfds, 2, -1) < 0) {
+  for (;;) {
+    if (poll(pfds, 2, -1) < 0) {
       if (toys.signal) handle_esc();
-      else sleep(1);
-
-      continue;
+      else perror_exit("poll");
     }
-    if(pfds[0].revents) {
-      len = read(0, TT.buff, DATABUFSIZE);
-      if(len > 0) write_server(len);
-      else return;
+    if (pfds[0].revents) {
+      if ((n = read(0, TT.buf, sizeof(TT.buf))) <= 0) xexit();
+      handle_user_input(n);
     }
-    if(pfds[1].revents) {
-      len = read(TT.sfd, TT.buff, DATABUFSIZE);
-      if(len > 0) read_server(len);
-      else {
-        printf("Connection closed by foreign host\r\n");
-        if(TT.term_ok) tcsetattr(0, TCSADRAIN, &TT.def_term);
-        exit(1);
-      }
+    if (pfds[1].revents) {
+      if ((n = read(TT.sock, TT.buf, sizeof(TT.buf))) <= 0)
+        error_exit("Connection closed by foreign host\r");
+      handle_server_output(n);
     }
+    if (TT.iac_len) flush_iac();
   }
 }