A zero return from BIO_read/BIO_write() could be retryable

A zero return from BIO_read()/BIO_write() could mean that an IO operation
is retryable. A zero return from SSL_read()/SSL_write() means that the
connection has been closed down (either cleanly or not). Therefore we
should not propagate a zero return value from BIO_read()/BIO_write() back
up the stack to SSL_read()/SSL_write(). This could result in a retryable
failure being treated as fatal.

Reviewed-by: Richard Levitte <levitte@openssl.org>

diff --git a/ssl/s23_pkt.c b/ssl/s23_pkt.c
index efc8647841bf21db77dd94608e55605988854d3c..5a63eff740db630d7c28cc118b3521150517bbc6 100644 (file)
--- a/ssl/s23_pkt.c
+++ b/ssl/s23_pkt.c
@@ -63,6 +63,12 @@
 #include <openssl/evp.h>
 #include <openssl/buffer.h>
 
+/*
+ * Return values are as per SSL_write(), i.e.
+ * >0 The number of read bytes
+ *  0 Failure (not retryable)
+ * <0 Failure (may be retryable)
+ */
 int ssl23_write_bytes(SSL *s)
 {
     int i, num, tot;
@@ -77,7 +83,7 @@ int ssl23_write_bytes(SSL *s)
         if (i <= 0) {
             s->init_off = tot;
             s->init_num = num;
-            return (i);
+            return -1;
         }
         s->rwstate = SSL_NOTHING;
         if (i == num)
@@ -88,7 +94,13 @@ int ssl23_write_bytes(SSL *s)
     }
 }
 
-/* return regularly only when we have read (at least) 'n' bytes */
+/* return regularly only when we have read (at least) 'n' bytes
+ * 
+ * Return values are as per SSL_read(), i.e.
+ * >0 The number of read bytes
+ *  0 Failure (not retryable)
+ * <0 Failure (may be retryable)
+ */
 int ssl23_read_bytes(SSL *s, int n)
 {
     unsigned char *p;
@@ -102,7 +114,7 @@ int ssl23_read_bytes(SSL *s, int n)
             j = BIO_read(s->rbio, (char *)&(p[s->packet_length]),
                          n - s->packet_length);
             if (j <= 0)
-                return (j);
+                return -1;
             s->rwstate = SSL_NOTHING;
             s->packet_length += j;
             if (s->packet_length >= (unsigned int)n)

diff --git a/ssl/s2_pkt.c b/ssl/s2_pkt.c
index 7a6188813431f345b2839a1b33873d99a272e3b1..394b4334793ac6137a416468677e5ab74a93b825 100644 (file)
--- a/ssl/s2_pkt.c
+++ b/ssl/s2_pkt.c
@@ -307,6 +307,12 @@ int ssl2_peek(SSL *s, void *buf, int len)
     return ssl2_read_internal(s, buf, len, 1);
 }
 
+/*
+ * Return values are as per SSL_read(), i.e.
+ * >0 The number of read bytes
+ *  0 Failure (not retryable)
+ * <0 Failure (may be retryable)
+ */
 static int read_n(SSL *s, unsigned int n, unsigned int max,
                   unsigned int extend)
 {
@@ -374,7 +380,7 @@ static int read_n(SSL *s, unsigned int n, unsigned int max,
 # endif
         if (i <= 0) {
             s->s2->rbuf_left += newb;
-            return (i);
+            return -1;
         }
         newb += i;
     }
@@ -441,6 +447,12 @@ int ssl2_write(SSL *s, const void *_buf, int len)
     }
 }
 
+/*
+ * Return values are as per SSL_write(), i.e.
+ * >0 The number of read bytes
+ *  0 Failure (not retryable)
+ * <0 Failure (may be retryable)
+ */
 static int write_pending(SSL *s, const unsigned char *buf, unsigned int len)
 {
     int i;
@@ -477,7 +489,7 @@ static int write_pending(SSL *s, const unsigned char *buf, unsigned int len)
             s->rwstate = SSL_NOTHING;
             return (s->s2->wpend_ret);
         } else if (i <= 0)
-            return (i);
+            return -1;
         s->s2->wpend_off += i;
         s->s2->wpend_len -= i;
     }

diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index be37ef0e50d8b00508cb0b5757522615aebc8050..7e3a7b480e7c4835ee32166c95922093a218d73c 100644 (file)
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -136,6 +136,12 @@ static int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
                          unsigned int len, int create_empty_fragment);
 static int ssl3_get_record(SSL *s);
 
+/*
+ * Return values are as per SSL_read(), i.e.
+ * >0 The number of read bytes
+ *  0 Failure (not retryable)
+ * <0 Failure (may be retryable)
+ */
 int ssl3_read_n(SSL *s, int n, int max, int extend)
 {
     /*
@@ -263,7 +269,7 @@ int ssl3_read_n(SSL *s, int n, int max, int extend)
             if (s->mode & SSL_MODE_RELEASE_BUFFERS && !SSL_IS_DTLS(s))
                 if (len + left == 0)
                     ssl3_release_read_buffer(s);
-            return (i);
+            return -1;
         }
         left += i;
         /*
@@ -1082,7 +1088,13 @@ static int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
     return -1;
 }
 
-/* if s->s3->wbuf.left != 0, we need to call this */
+/* if s->s3->wbuf.left != 0, we need to call this
+ * 
+ * Return values are as per SSL_write(), i.e.
+ * >0 The number of read bytes
+ *  0 Failure (not retryable)
+ * <0 Failure (may be retryable)
+ */
 int ssl3_write_pending(SSL *s, int type, const unsigned char *buf,
                        unsigned int len)
 {
@@ -1122,7 +1134,7 @@ int ssl3_write_pending(SSL *s, int type, const unsigned char *buf,
                  */
                 wb->left = 0;
             }
-            return (i);
+            return -1;
         }
         wb->offset += i;
         wb->left -= i;