aio: Fix eventloop and eof for ssl connections

We can't use feof() and 'buffering none' on ssl connections.
Instead we have to get eof from the ssl layer, and provide
special handling for buffering in the eventloop.

For eof, add ssl_eof() and detect SSL_read() results that indicate
eof to set AIO_EOF in flags.

For buffering, add 'read -pending' that will read, and then immediately
read any buffered data so that the 'readable' event will trigger next
time.

Signed-off-by: Steve Bennett <steveb@workware.net.au>

4 files changed, 131 insertions, 44 deletions

diff --git a/jim-aio.c b/jim-aio.c
index 06d9fa8..0376de4 100644
--- a/jim-aio.c
+++ b/jim-aio.c
@@ -95,6 +95,7 @@
 
 #define AIO_KEEPOPEN 1
 #define AIO_NODELETE 2
+#define AIO_EOF 4
 
 #if defined(JIM_IPV6)
 #define IPV6 1
@@ -150,6 +151,8 @@ typedef struct {
     int (*error)(const struct AioFile *af);
     const char *(*strerror)(struct AioFile *af);
     int (*verify)(struct AioFile *af);
+    int (*eof)(struct AioFile *af);
+    int (*pending)(struct AioFile *af);
 } JimAioFopsType;
 
 typedef struct AioFile
@@ -157,7 +160,7 @@ typedef struct AioFile
     FILE *fp;
     Jim_Obj *filename;
     int type;
-    int openFlags;              /* AIO_KEEPOPEN? keep FILE* */
+    int flags;              /* AIO_KEEPOPEN? keep FILE* */
     int fd;
     Jim_Obj *rEvent;
     Jim_Obj *wEvent;
@@ -210,13 +213,20 @@ static const char *stdio_strerror(struct AioFile *af)
     return strerror(errno);
 }
 
+static int stdio_eof(struct AioFile *af)
+{
+    return feof(af->fp);
+}
+
 static const JimAioFopsType stdio_fops = {
     stdio_writer,
     stdio_reader,
     stdio_getline,
     stdio_error,
     stdio_strerror,
-    NULL
+    NULL, /* verify */
+    stdio_eof,
+    NULL, /* pending */
 };
 
 #if defined(JIM_SSL) && !defined(JIM_BOOTSTRAP)
@@ -228,35 +238,64 @@ static int ssl_writer(struct AioFile *af, const char *buf, int len)
     return SSL_write(af->ssl, buf, len);
 }
 
+static int ssl_pending(struct AioFile *af)
+{
+    return SSL_pending(af->ssl);
+}
+
 static int ssl_reader(struct AioFile *af, char *buf, int len)
 {
-    return SSL_read(af->ssl, buf, len);
+    int ret = SSL_read(af->ssl, buf, len);
+    switch (SSL_get_error(af->ssl, ret)) {
+    case SSL_ERROR_NONE:
+        return ret;
+    case SSL_ERROR_SYSCALL:
+    case SSL_ERROR_ZERO_RETURN:
+        if (errno != EAGAIN) {
+            af->flags |= AIO_EOF;
+        }
+        return 0;
+    case SSL_ERROR_SSL:
+    default:
+        if (errno == EAGAIN) {
+            return 0;
+        }
+        af->flags |= AIO_EOF;
+        return -1;
+    }
+}
+
+static int ssl_eof(struct AioFile *af)
+{
+    return (af->flags & AIO_EOF);
 }
 
 static const char *ssl_getline(struct AioFile *af, char *buf, int len)
 {
     size_t i;
-    for (i = 0; i < len + 1; i++) {
-        if (SSL_read(af->ssl, &buf[i], 1) != 1) {
-            if (i == 0) {
-                return NULL;
-            }
+    for (i = 0; i < len - 1 && !ssl_eof(af); i++) {
+        int ret = ssl_reader(af, &buf[i], 1);
+        if (ret != 1) {
             break;
         }
         if (buf[i] == '\n') {
+            i++;
             break;
         }
     }
     buf[i] = '\0';
+    if (i == 0 && ssl_eof(af)) {
+        return NULL;
+    }
     return buf;
 }
 
 static int ssl_error(const struct AioFile *af)
 {
-    if (ERR_peek_error() == 0) {
-        return JIM_OK;
+    int ret = SSL_get_error(af->ssl, 0);
+    if (ret == SSL_ERROR_SYSCALL || ret == 0) {
+        return stdio_error(af);
     }
-
     return JIM_ERR;
 }
 
@@ -295,7 +334,9 @@ static const JimAioFopsType ssl_fops = {
     ssl_getline,
     ssl_error,
     ssl_strerror,
-    ssl_verify
+    ssl_verify,
+    ssl_eof,
+    ssl_pending,
 };
 #endif /* JIM_BOOTSTRAP */
 
@@ -605,7 +646,7 @@ static void JimAioDelProc(Jim_Interp *interp, void *privData)
     JIM_NOTUSED(interp);
 
 #if UNIX_SOCKETS
-    if (af->addr_family == PF_UNIX && (af->openFlags & AIO_NODELETE) == 0) {
+    if (af->addr_family == PF_UNIX && (af->flags & AIO_NODELETE) == 0) {
         /* If this is bound, delete the socket file now */
         Jim_Obj *filenameObj = aio_sockname(interp, af);
         if (filenameObj) {
@@ -629,7 +670,7 @@ static void JimAioDelProc(Jim_Interp *interp, void *privData)
         SSL_free(af->ssl);
     }
 #endif
-    if (!(af->openFlags & AIO_KEEPOPEN)) {
+    if (!(af->flags & AIO_KEEPOPEN)) {
         fclose(af->fp);
     }
 
@@ -642,22 +683,42 @@ static int aio_cmd_read(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
     char buf[AIO_BUF_LEN];
     Jim_Obj *objPtr;
     int nonewline = 0;
+    int pending = 0;
     jim_wide neededLen = -1;         /* -1 is "read as much as possible" */
+    static const char * const options[] = { "-pending", "-nonewline", NULL };
+    enum { OPT_PENDING, OPT_NONEWLINE };
+    int option;
 
-    if (argc && Jim_CompareStringImmediate(interp, argv[0], "-nonewline")) {
-        nonewline = 1;
-        argv++;
-        argc--;
-    }
-    if (argc == 1) {
-        if (Jim_GetWide(interp, argv[0], &neededLen) != JIM_OK)
-            return JIM_ERR;
-        if (neededLen < 0) {
-            Jim_SetResultString(interp, "invalid parameter: negative len", -1);
-            return JIM_ERR;
+    if (argc) {
+        if (*Jim_String(argv[0]) == '-') {
+            if (Jim_GetEnum(interp, argv[0], options, &option, NULL, JIM_ERRMSG) != JIM_OK) {
+                return JIM_ERR;
+            }
+            switch (option) {
+                case OPT_PENDING:
+                    if (!af->fops->pending) {
+                        Jim_SetResultString(interp, "-pending not supported on this connection type", -1);
+                        return JIM_ERR;
+                    }
+                    pending++;
+                    break;
+                case OPT_NONEWLINE:
+                    nonewline++;
+                    break;
+            }
         }
+        else {
+            if (Jim_GetWide(interp, argv[0], &neededLen) != JIM_OK)
+                return JIM_ERR;
+            if (neededLen < 0) {
+                Jim_SetResultString(interp, "invalid parameter: negative len", -1);
+                return JIM_ERR;
+            }
+        }
+        argc--;
+        argv++;
     }
-    else if (argc) {
+    if (argc) {
         return -1;
     }
     objPtr = Jim_NewStringObj(interp, NULL, 0);
@@ -671,15 +732,22 @@ static int aio_cmd_read(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
         else {
             readlen = (neededLen > AIO_BUF_LEN ? AIO_BUF_LEN : neededLen);
         }
-        retval = af->fops->reader(af, buf, readlen);
+        retval = af->fops->reader(af, buf, pending ? 1 : readlen);
         if (retval > 0) {
             Jim_AppendString(interp, objPtr, buf, retval);
             if (neededLen != -1) {
                 neededLen -= retval;
             }
+            else if (pending) {
+                /* If pending was specified, after we do the initial read,
+                 * we do a second read to fetch any buffered data
+                 */
+                neededLen = af->fops->pending(af);
+            }
         }
-        if (retval != readlen)
+        if (retval <= 0) {
             break;
+        }
     }
     /* Check for error conditions */
     if (JimCheckStreamError(interp, af)) {
@@ -823,7 +891,7 @@ static int aio_cmd_gets(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
 
         len = Jim_Length(objPtr);
 
-        if (len == 0 && feof(af->fp)) {
+        if (len == 0 && af->fops->eof(af)) {
             /* On EOF returns -1 if varName was specified */
             len = -1;
         }
@@ -1016,7 +1084,7 @@ static int aio_cmd_eof(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
 {
     AioFile *af = Jim_CmdPrivData(interp);
 
-    Jim_SetResultInt(interp, !!feof(af->fp));
+    Jim_SetResultInt(interp, !!af->fops->eof(af));
     return JIM_OK;
 }
 
@@ -1046,7 +1114,7 @@ static int aio_cmd_close(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
 #if UNIX_SOCKETS
             case OPT_NODELETE:
                 if (af->addr_family == PF_UNIX) {
-                    af->openFlags |= AIO_NODELETE;
+                    af->flags |= AIO_NODELETE;
                     break;
                 }
                 /* fall through */
@@ -1542,7 +1610,7 @@ static int aio_cmd_tty(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
 
 static const jim_subcmd_type aio_command_table[] = {
     {   "read",
-        "?-nonewline? ?len?",
+        "?-nonewline|-pending|len?",
         aio_cmd_read,
         0,
         2,
@@ -1889,7 +1957,7 @@ static AioFile *JimMakeChannel(Jim_Interp *interp, FILE *fh, int fd, Jim_Obj *fi
     memset(af, 0, sizeof(*af));
     af->fp = fh;
     af->filename = filename;
-    af->openFlags = flags;
+    af->flags = flags;
 #ifndef JIM_ANSIC
     af->fd = fileno(fh);
 #ifdef FD_CLOEXEC
diff --git a/jim_tcl.txt b/jim_tcl.txt
index 0ba2e5b..d0f3b25 100644
--- a/jim_tcl.txt
+++ b/jim_tcl.txt
@@ -3574,6 +3574,10 @@ read
 
 +'fileId' *read* 'numBytes'+
 
++*read* ?*-pending*? 'fileId'+
+
++'fileId' *read* ?*-pending*?+
+
 In the first form, all of the remaining bytes are read from the file
 given by +'fileId'+; they are returned as the result of the command.
 If the +-nonewline+ switch is specified then the last
@@ -3584,6 +3588,21 @@ exactly this many bytes will be read and returned, unless there are fewer than
 +'numBytes'+ bytes left in the file; in this case, all the remaining
 bytes are returned.
 
+The third form is currently only useful with SSL sockets. It reads at least 1 byte
+and then any additional data that is buffered. This allows for use in an event handler.
+e.g.
+
+----
+    $sock readable {
+        set buf [$sock read -pending]
+    }
+----
+
+This is necessary because otherwise pending data may be buffered, but
+the underlying socket will not be marked 'readable'. This featured is not
+currently supported for regular sockets, and so these sockets must be
+set to unbufferred (+$sock buffering false+) to work in an event loop.
+
 +'fileId'+ must be +stdin+ or the return value from a previous call
 to `open`; it must refer to a file that was opened for reading.
 
@@ -4753,8 +4772,8 @@ aio
 +$handle *puts ?-nonewline?* 'str'+::
     Write the string, with newline unless -nonewline
 
-+$handle *read ?-nonewline?* '?len?'+::
-    Read and return bytes from the stream. To eof if no len.
++$handle *read ?-nonewline|-pending*|len?'+::
+    Read and return bytes from the stream. To eof if no len. See `read`.
 
 +$handle *recvfrom* 'maxlen ?addrvar?'+::
     Receives a message from the handle via recvfrom(2) and returns it.
diff --git a/tests/aio.test b/tests/aio.test
index 501b9a6..361acdc 100644
--- a/tests/aio.test
+++ b/tests/aio.test
@@ -58,7 +58,7 @@ test aio-1.9 {seek bad pos} -body {
 
 test aio-2.1 {read usage} -body {
 	$f read -nonoption
-} -returnCodes error -result {expected integer but got "-nonoption"}
+} -returnCodes error -result {bad option "-nonoption": must be -nonewline, or -pending}
 
 test aio-2.2 {read usage} -body {
 	$f read badint
@@ -70,7 +70,11 @@ test aio-2.3 {read -ve len} -body {
 
 test aio-2.4 {read too many args} -body {
 	$f read 20 extra
-} -returnCodes error -match glob -result {wrong # args: should be "* read ?-nonewline? ?len?"}
+} -returnCodes error -match glob -result {wrong # args: should be "* read ?-nonewline|-pending|len?"}
+
+test aio-2.5 {read -pending on non-ssl} -body {
+	$f read -pending
+} -returnCodes error -result {-pending not supported on this connection type}
 
 test aio-3.1 {copy to invalid fh} -body {
 	$f copy lambda
diff --git a/tests/ssl.test b/tests/ssl.test
index a15c4d6..f07391e 100644
--- a/tests/ssl.test
+++ b/tests/ssl.test
@@ -2,7 +2,6 @@ source [file dirname [info script]]/testing.tcl
 
 needs constraint jim
 needs cmd socket
-needs cmd alarm
 needs cmd os.fork
 testCmdConstraints load_ssl_certs
 
@@ -16,11 +15,12 @@ set s [socket stream.server 1443]
 if {[os.fork] == 0} {
 	# child
 	set c [[socket stream [$s sockname]] ssl]
-	$c buffering none
 	$s close
 	sleep 0.25
 	$c readable {
-		set buf [$c read 1]
+		# when we read we need to also read any pending data,
+		# otherwise readable won't retrigger
+		set buf [$c read -pending]
 		if {[string length $buf] == 0} {
 			incr ssldone
 			$c close
@@ -47,9 +47,6 @@ test ssl-1.1 {puts/gets} {
 	$cs gets
 } hello
 
-# XXX this test does not work because of the interaction between
-# ssl buffering and readable
-alarm 1
 test ssl-1.2 {puts/gets} {
 	$cs puts -nonewline again
 	lmap p [range 5] {
@@ -57,7 +54,6 @@ test ssl-1.2 {puts/gets} {
 		set c
 	}
 } {a g a i n}
-alarm 0
 
 test ssl-2.1 {https to google.com, gets} -body {
 	set c [[socket stream www.google.com:443] ssl]