commit 9a38e924aafa96b4f1d7495ce140ba06b956854f
author David Benjamin <davidben@chromium.org> Thu Jan 22 16:06:11 2015 -0500
committer Adam Langley <agl@google.com> Thu Jan 22 22:01:35 2015 +0000
tree f9e216ab4b8b939f08e534aaf08b6ca06a65b366
parent 1e52ecac4dca31d0007a76f0960ea4c8c79ee094

Return SSL_ERROR_SYSCALL on unclean EOF.

This regressed in fcf25833bc4d057377da1ea755f57b2f5126e555. 0 return code on
unclean shutdown means the underlying BIO returned EOF, didn't push any error
code, but we haven't seen close_notify yet. The intent seems to be that you go
check errno or some BIO-specific equivalent if you care about close_notify.

Make sure test code routes all SSL_read return codes through SSL_get_error
since that's supposed to work in all cases.

(Note that rv == 0 can still give SSL_ERROR_SSL if the error queue is not
empty.)

Change-Id: I45bf9614573f876d93419ce169a4e0d9ceea9052
Reviewed-on: https://boringssl-review.googlesource.com/2981
Reviewed-by: Adam Langley <agl@google.com>

ssl/ssl_lib.c

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 311c1e4..c3b95d5 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2251,13 +2251,18 @@
     return SSL_ERROR_SSL;
   }
 
-  if (i == 0 && (s->shutdown & SSL_RECEIVED_SHUTDOWN) &&
-      (s->s3->warn_alert == SSL_AD_CLOSE_NOTIFY)) {
-    return SSL_ERROR_ZERO_RETURN;
+  if (i == 0) {
+    if ((s->shutdown & SSL_RECEIVED_SHUTDOWN) &&
+        (s->s3->warn_alert == SSL_AD_CLOSE_NOTIFY)) {
+      /* The socket was cleanly shut down with a close_notify. */
+      return SSL_ERROR_ZERO_RETURN;
+    }
+    /* An EOF was observed which violates the protocol, and the underlying
+     * transport does not participate in the error queue. Bubble up to the
+     * caller. */
+    return SSL_ERROR_SYSCALL;
   }
 
-  assert(i < 0);
-
   if (SSL_want_session(s)) {
     return SSL_ERROR_PENDING_SESSION;
   }

ssl/test/bssl_shim.cc

diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 4fee8dc..19355af 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -656,25 +656,41 @@
       do {
         n = SSL_read(ssl, buf, sizeof(buf));
       } while (config->async && retry_async(ssl, n, bio));
-      if (n < 0) {
+      int err = SSL_get_error(ssl, n);
+      if (err == SSL_ERROR_ZERO_RETURN ||
+          (n == 0 && err == SSL_ERROR_SYSCALL)) {
+        if (n != 0) {
+          fprintf(stderr, "Invalid SSL_get_error output\n");
+          return 3;
+        }
+        /* Accept shutdowns with or without close_notify.
+         * TODO(davidben): Write tests which distinguish these two cases. */
+        break;
+      } else if (err != SSL_ERROR_NONE) {
+        if (n > 0) {
+          fprintf(stderr, "Invalid SSL_get_error output\n");
+          return 3;
+        }
         SSL_free(ssl);
         BIO_print_errors_fp(stdout);
         return 3;
-      } else if (n == 0) {
-        break;
-      } else {
-        for (int i = 0; i < n; i++) {
-          buf[i] ^= 0xff;
-        }
-        int w;
-        do {
-          w = SSL_write(ssl, buf, n);
-        } while (config->async && retry_async(ssl, w, bio));
-        if (w != n) {
-          SSL_free(ssl);
-          BIO_print_errors_fp(stdout);
-          return 4;
-        }
+      }
+      /* Successfully read data. */
+      if (n <= 0) {
+        fprintf(stderr, "Invalid SSL_get_error output\n");
+        return 3;
+      }
+      for (int i = 0; i < n; i++) {
+        buf[i] ^= 0xff;
+      }
+      int w;
+      do {
+        w = SSL_write(ssl, buf, n);
+      } while (config->async && retry_async(ssl, w, bio));
+      if (w != n) {
+        SSL_free(ssl);
+        BIO_print_errors_fp(stdout);
+        return 4;
       }
     }
   }