Bug 1385203 - Ensure wrBuf is empty when not in use, r=ekr

4 files changed, 41 insertions, 16 deletions

diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc
index 5e5c55604..4bc6e60ab 100644
--- a/gtests/ssl_gtest/ssl_loopback_unittest.cc
+++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc
@@ -430,11 +430,27 @@ TEST_F(TlsConnectStreamTls13, Tls13FailedWriteSecondFlight) {
   StartConnect();
   client_->Handshake();
   server_->Handshake();  // Send first flight.
-  client_->adapter()->CloseWrites();
+  client_->adapter()->SetWriteError(PR_IO_ERROR);
   client_->Handshake();  // This will get an error, but shouldn't crash.
   client_->CheckErrorCode(SSL_ERROR_SOCKET_WRITE_FAILURE);
 }
 
+TEST_P(TlsConnectDatagram, BlockedWrite) {
+  Connect();
+
+  // Mark the socket as blocked.
+  client_->adapter()->SetWriteError(PR_WOULD_BLOCK_ERROR);
+  static const uint8_t data[] = {1, 2, 3};
+  int32_t rv = PR_Write(client_->ssl_fd(), data, sizeof(data));
+  EXPECT_GT(0, rv);
+  EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
+
+  // Remove the write error and though the previous write failed, future reads
+  // and writes should just work as if it never happened.
+  client_->adapter()->SetWriteError(0);
+  SendReceive();
+}
+
 TEST_F(TlsConnectTest, ConnectSSLv3) {
   ConfigureVersion(SSL_LIBRARY_VERSION_3_0);
   EnableOnlyStaticRsaCiphers();
diff --git a/gtests/ssl_gtest/test_io.cc b/gtests/ssl_gtest/test_io.cc
index addef09f4..adcdbfbaf 100644
--- a/gtests/ssl_gtest/test_io.cc
+++ b/gtests/ssl_gtest/test_io.cc
@@ -98,8 +98,13 @@ int32_t DummyPrSocket::Recv(PRFileDesc *f, void *buf, int32_t buflen,
 }
 
 int32_t DummyPrSocket::Write(PRFileDesc *f, const void *buf, int32_t length) {
+  if (write_error_) {
+    PR_SetError(write_error_, 0);
+    return -1;
+  }
+
   auto peer = peer_.lock();
-  if (!peer || !writeable_) {
+  if (!peer) {
     PR_SetError(PR_IO_ERROR, 0);
     return -1;
   }
diff --git a/gtests/ssl_gtest/test_io.h b/gtests/ssl_gtest/test_io.h
index f2498904c..469d90a7c 100644
--- a/gtests/ssl_gtest/test_io.h
+++ b/gtests/ssl_gtest/test_io.h
@@ -65,7 +65,7 @@ class DummyPrSocket : public DummyIOLayerMethods {
         peer_(),
         input_(),
         filter_(nullptr),
-        writeable_(true) {}
+        write_error_(0) {}
   virtual ~DummyPrSocket() {}
 
   // Create a file descriptor that will reference this object.  The fd must not
@@ -83,7 +83,7 @@ class DummyPrSocket : public DummyIOLayerMethods {
   int32_t Recv(PRFileDesc* f, void* buf, int32_t buflen, int32_t flags,
                PRIntervalTime to) override;
   int32_t Write(PRFileDesc* f, const void* buf, int32_t length) override;
-  void CloseWrites() { writeable_ = false; }
+  void SetWriteError(PRErrorCode code) { write_error_ = code; }
 
   SSLProtocolVariant variant() const { return variant_; }
   bool readable() const { return !input_.empty(); }
@@ -110,7 +110,7 @@ class DummyPrSocket : public DummyIOLayerMethods {
   std::weak_ptr<DummyPrSocket> peer_;
   std::queue<Packet> input_;
   std::shared_ptr<PacketFilter> filter_;
-  bool writeable_;
+  PRErrorCode write_error_;
 };
 
 // Marker interface.
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 1dfa3a44e..9c9db093f 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -2329,6 +2329,7 @@ ssl3_SendRecord(sslSocket *ss,
     PRINT_BUF(50, (ss, "Send record (plain text)", pIn, nIn));
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
+    PORT_Assert(SSL_BUFFER_LEN(wrBuf) == 0);
 
     if (ss->ssl3.fatalAlertSent) {
         SSL_TRC(3, ("%d: SSL3[%d] Suppress write, fatal alert already sent",
@@ -2354,6 +2355,7 @@ ssl3_SendRecord(sslSocket *ss,
 
     while (nIn > 0) {
         unsigned int written = 0;
+        PRInt32 sent;
 
         ssl_GetSpecReadLock(ss);
         rv = ssl_ProtectNextRecord(ss, spec, type, pIn, nIn, &written);
@@ -2383,42 +2385,39 @@ ssl3_SendRecord(sslSocket *ss,
                                    SSL_BUFFER_LEN(wrBuf));
             if (rv != SECSuccess) {
                 /* presumably a memory error, SEC_ERROR_NO_MEMORY */
-                return SECFailure;
+                goto loser;
             }
 
             if (!(flags & ssl_SEND_FLAG_FORCE_INTO_BUFFER)) {
-                PRInt32 sent;
                 ss->handshakeBegun = 1;
                 sent = ssl_SendSavedWriteData(ss);
                 if (sent < 0 && PR_GetError() != PR_WOULD_BLOCK_ERROR) {
                     ssl_MapLowLevelError(SSL_ERROR_SOCKET_WRITE_FAILURE);
-                    return SECFailure;
+                    goto loser;
                 }
                 if (ss->pendingBuf.len) {
                     flags |= ssl_SEND_FLAG_FORCE_INTO_BUFFER;
                 }
             }
         } else {
-            PRInt32 sent;
-
             PORT_Assert(SSL_BUFFER_LEN(wrBuf) > 0);
             ss->handshakeBegun = 1;
             sent = ssl_DefSend(ss, SSL_BUFFER_BASE(wrBuf),
                                SSL_BUFFER_LEN(wrBuf),
                                flags & ~ssl_SEND_FLAG_MASK);
             if (sent < 0) {
-                if (PR_GetError() != PR_WOULD_BLOCK_ERROR) {
+                if (PORT_GetError() != PR_WOULD_BLOCK_ERROR) {
                     ssl_MapLowLevelError(SSL_ERROR_SOCKET_WRITE_FAILURE);
-                    return SECFailure;
+                    goto loser;
                 }
                 /* we got PR_WOULD_BLOCK_ERROR, which means none was sent. */
                 sent = 0;
             }
-            if (SSL_BUFFER_LEN(wrBuf) > sent) {
+            if (SSL_BUFFER_LEN(wrBuf) > (unsigned int)sent) {
                 if (IS_DTLS(ss)) {
                     /* DTLS just says no in this case. No buffering */
-                    PR_SetError(PR_WOULD_BLOCK_ERROR, 0);
-                    return SECFailure;
+                    PORT_SetError(PR_WOULD_BLOCK_ERROR);
+                    goto loser;
                 }
                 /* now take all the remaining unsent new ciphertext and
                  * append it to the buffer of previously unsent ciphertext.
@@ -2427,7 +2426,7 @@ ssl3_SendRecord(sslSocket *ss,
                                        SSL_BUFFER_LEN(wrBuf) - sent);
                 if (rv != SECSuccess) {
                     /* presumably a memory error, SEC_ERROR_NO_MEMORY */
-                    return SECFailure;
+                    goto loser;
                 }
             }
         }
@@ -2435,6 +2434,11 @@ ssl3_SendRecord(sslSocket *ss,
         totalSent += written;
     }
     return totalSent;
+
+loser:
+    /* Don't leave bits of buffer lying around. */
+    wrBuf->len = 0;
+    return -1;
 }
 
 #define SSL3_PENDING_HIGH_WATER 1024