Clarify logic in BIO_*printf functions

The static function dynamically allocates an output buffer if the output
grows larger than the static buffer that is normally used. The original
logic implied that |currlen| could be greater than |maxlen| which is
incorrect (and if so would cause a buffer overrun). Also the original
logic would call OPENSSL_malloc to create a dynamic buffer equal to the
size of the static buffer, and then immediately call OPENSSL_realloc to
make it bigger, rather than just creating a buffer than was big enough in
the first place. Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot
Oberoi (Int3 Solutions) for reporting this issue.

Reviewed-by: Andy Polyakov <appro@openssl.org>

diff --git a/crypto/bio/b_print.c b/crypto/bio/b_print.c
index 452e5cfd95909e931490a108cf50df91df5c5d76..7c81e25d482cf1d93146f5725750445aa56be018 100644 (file)
--- a/crypto/bio/b_print.c
+++ b/crypto/bio/b_print.c
@@ -704,32 +704,29 @@ doapr_outch(char **sbuffer,
     /* If we haven't at least one buffer, someone has doe a big booboo */
     assert(*sbuffer != NULL || buffer != NULL);
 
     /* If we haven't at least one buffer, someone has doe a big booboo */
     assert(*sbuffer != NULL || buffer != NULL);
 

-    if (buffer) {
-        while (*currlen >= *maxlen) {
-            if (*buffer == NULL) {
-                if (*maxlen == 0)
-                    *maxlen = 1024;
-                *buffer = OPENSSL_malloc(*maxlen);
-                if (!*buffer) {
-                    /* Panic! Can't really do anything sensible. Just return */
-                    return;
-                }
-                if (*currlen > 0) {
-                    assert(*sbuffer != NULL);
-                    memcpy(*buffer, *sbuffer, *currlen);
-                }
-                *sbuffer = NULL;
-            } else {
-                *maxlen += 1024;
-                *buffer = OPENSSL_realloc(*buffer, *maxlen);
-                if (!*buffer) {
-                    /* Panic! Can't really do anything sensible. Just return */
-                    return;
-                }
+    /* |currlen| must always be <= |*maxlen| */
+    assert(*currlen <= *maxlen);
+
+    if (buffer && *currlen == *maxlen) {
+        *maxlen += 1024;
+        if (*buffer == NULL) {
+            *buffer = OPENSSL_malloc(*maxlen);
+            if (!*buffer) {
+                /* Panic! Can't really do anything sensible. Just return */
+                return;
+            }
+            if (*currlen > 0) {
+                assert(*sbuffer != NULL);
+                memcpy(*buffer, *sbuffer, *currlen);
+            }
+            *sbuffer = NULL;
+        } else {
+            *buffer = OPENSSL_realloc(*buffer, *maxlen);
+            if (!*buffer) {
+                /* Panic! Can't really do anything sensible. Just return */
+                return;

             }
         }
             }
         }

-        /* What to do if *buffer is NULL? */
-        assert(*sbuffer != NULL || *buffer != NULL);

     }
 
     if (*currlen < *maxlen) {
     }
 
     if (*currlen < *maxlen) {