Bug 1765383 - GatherBuffer: Reduced plaintext buffer allocations by allocating it on initialization. Replaced redundant code with assert. Debug builds: Added buffer freeing/allocation for each record. r=djackson

Differential Revision: https://phabricator.services.mozilla.com/D144034

3 files changed, 47 insertions, 18 deletions

diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 09d7f098b..de5eb3e97 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -13372,17 +13372,17 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
         return SECFailure;
     }
 
-    if (plaintext->space < cTextSizeLimit) {
-        rv = sslBuffer_Grow(plaintext, cTextSizeLimit);
-        if (rv != SECSuccess) {
-            ssl_ReleaseSpecReadLock(ss); /*************************/
-            SSL_DBG(("%d: SSL3[%d]: HandleRecord, tried to get %d bytes",
-                     SSL_GETPID(), ss->fd, cTextSizeLimit));
-            /* sslBuffer_Grow has set a memory error code. */
-            /* Perhaps we should send an alert. (but we have no memory!) */
-            return SECFailure;
-        }
-    }
+#ifdef DEBUG
+    /* In debug builds the gather buffers are freed after the handling of each
+     * record for advanced ASAN coverage. Allocate the buffer again to the
+     * maximum possibly needed size as on gather initialization in
+     * ssl3gthr.c/ssl3_InitGather(). */
+    PR_ASSERT(sslBuffer_Grow(plaintext, TLS_1_2_MAX_CTEXT_LENGTH) == SECSuccess);
+#endif
+    /* This replaces a dynamic plaintext buffer size check, since the buffer is
+     * allocated to the maximum size in ssl3gthr.c/ssl3_InitGather(). The buffer
+     * was always grown to the maximum size at first record gathering before. */
+    PR_ASSERT(plaintext->space >= cTextSizeLimit);
 
     /* Most record types aside from protected TLS 1.3 records carry the content
      * type in the first octet. TLS 1.3 will override this value later. */
@@ -13390,7 +13390,6 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
     /* Encrypted application data records could arrive before the handshake
      * completes in DTLS 1.3. These can look like valid TLS 1.2 application_data
      * records in epoch 0, which is never valid. Pretend they didn't decrypt. */
-
     if (spec->epoch == 0 && ((IS_DTLS(ss) &&
                               dtls_IsDtls13Ciphertext(0, rType)) ||
                              rType == ssl_ct_application_data)) {
@@ -13526,8 +13525,18 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
         return SECFailure;
     }
 
-    return ssl3_HandleNonApplicationData(ss, rType, epoch, cText->seqNum,
-                                         plaintext);
+    rv = ssl3_HandleNonApplicationData(ss, rType, epoch, cText->seqNum,
+                                       plaintext);
+
+#ifdef DEBUG
+    /* In Debug builds free and zero gather plaintext buffer after its content
+     * has been used/copied for advanced ASAN coverage/utilization.
+     * This frees buffer for non application data records, for application data
+     * records it is freed in sslsecur.c/DoRecv(). */
+    sslBuffer_Clear(&ss->gs.buf);
+#endif
+
+    return rv;
 }
 
 /*
diff --git a/lib/ssl/ssl3gthr.c b/lib/ssl/ssl3gthr.c
index 08cbe7fd8..b87454d34 100644
--- a/lib/ssl/ssl3gthr.c
+++ b/lib/ssl/ssl3gthr.c
@@ -26,16 +26,16 @@ typedef struct ssl2GatherStr ssl2Gather;
 SECStatus
 ssl3_InitGather(sslGather *gs)
 {
-    SECStatus status;
-
     gs->state = GS_INIT;
     gs->writeOffset = 0;
     gs->readOffset = 0;
     gs->dtlsPacketOffset = 0;
     gs->dtlsPacket.len = 0;
     gs->rejectV2Records = PR_FALSE;
-    status = sslBuffer_Grow(&gs->buf, 4096);
-    return status;
+    /* Allocate plaintext buffer to maximum possibly needed size. It needs to
+     * be larger than recordSizeLimit for TLS 1.0 and 1.1 compatability.
+     * The TLS 1.2 ciphertext is larger than the TLS 1.3 ciphertext. */
+    return sslBuffer_Grow(&gs->buf, TLS_1_2_MAX_CTEXT_LENGTH);
 }
 
 /* Caller must hold RecvBufLock. */
@@ -560,6 +560,15 @@ ssl3_GatherCompleteHandshake(sslSocket *ss, int flags)
             cText.buf = &ss->gs.inbuf;
             rv = ssl3_HandleRecord(ss, &cText);
         }
+
+#ifdef DEBUG
+        /* In Debug builds free gather ciphertext buffer after each decryption
+         * for advanced ASAN coverage/utilization. The buffer content has been
+         * used at this point, ssl3_HandleRecord() and thereby the decryption
+         * functions are only called from this point of the implementation. */
+        sslBuffer_Clear(&ss->gs.inbuf);
+#endif
+
         if (rv < 0) {
             return ss->recvdCloseNotify ? 0 : rv;
         }
diff --git a/lib/ssl/sslsecur.c b/lib/ssl/sslsecur.c
index 2c9a4dbf9..281ea927a 100644
--- a/lib/ssl/sslsecur.c
+++ b/lib/ssl/sslsecur.c
@@ -551,6 +551,17 @@ DoRecv(sslSocket *ss, unsigned char *out, int len, int flags)
     PORT_Assert(ss->gs.readOffset <= ss->gs.writeOffset);
     rv = amount;
 
+#ifdef DEBUG
+    /* In Debug builds free and zero gather plaintext buffer after its content
+     * has been used/copied for advanced ASAN coverage/utilization.
+     * This frees the buffer after reception of application data,
+     * non-application data is freed at the end of
+     * ssl3con.c/ssl3_HandleRecord(). */
+    if (ss->gs.writeOffset == ss->gs.readOffset) {
+        sslBuffer_Clear(&ss->gs.buf);
+    }
+#endif
+
     SSL_TRC(30, ("%d: SSL[%d]: amount=%d available=%d",
                  SSL_GETPID(), ss->fd, amount, available));
     PRINT_BUF(4, (ss, "DoRecv receiving plaintext:", out, amount));