Fix BIO_printf crash on Mac.

A single va_list may not be used twice. Nothing calls BIO_vprintf and it just
(v)snprintfs into a buffer anyway, so remove it. If it's actually needed, we
can fiddle with va_copy and the lack of it in C89 later, but anything that
actually cares can just assemble the output externally.

Add a test in bio_test.c.

BUG=399546

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

2 files changed, 65 insertions, 18 deletions

diff --git a/crypto/bio/bio_test.c b/crypto/bio/bio_test.c
index 8b3c129d..a8e46abd 100644
--- a/crypto/bio/bio_test.c
+++ b/crypto/bio/bio_test.c
@@ -17,6 +17,7 @@
 #include <arpa/inet.h>
 #include <fcntl.h>
 #include <netinet/in.h>
+#include <string.h>
 #include <sys/socket.h>
 #include <unistd.h>
 
@@ -93,6 +94,56 @@ static int test_socket_connect() {
   return 1;
 }
 
+static int test_printf() {
+  /* Test a short output, a very long one, and various sizes around
+   * 256 (the size of the buffer) to ensure edge cases are correct. */
+  static const size_t kLengths[] = { 5, 250, 251, 252, 253, 254, 1023 };
+  BIO *bio;
+  char string[1024];
+  int ret;
+  const uint8_t *contents;
+  size_t len;
+
+  bio = BIO_new(BIO_s_mem());
+  if (!bio) {
+    fprintf(stderr, "BIO_new failed\n");
+    return 0;
+  }
+
+  for (size_t i = 0; i < sizeof(kLengths) / sizeof(kLengths[0]); i++) {
+    if (kLengths[i] >= sizeof(string)) {
+      fprintf(stderr, "Bad test string length\n");
+      return 0;
+    }
+    memset(string, 'a', sizeof(string));
+    string[kLengths[i]] = '\0';
+
+    ret = BIO_printf(bio, "test %s", string);
+    if (ret != 5 + kLengths[i]) {
+      fprintf(stderr, "BIO_printf failed\n");
+      return 0;
+    }
+    if (!BIO_mem_contents(bio, &contents, &len)) {
+      fprintf(stderr, "BIO_mem_contents failed\n");
+      return 0;
+    }
+    if (len != 5 + kLengths[i] ||
+        strncmp((const char *)contents, "test ", 5) != 0 ||
+        strncmp((const char *)contents + 5, string, kLengths[i]) != 0) {
+      fprintf(stderr, "Contents did not match: %.*s\n", (int)len, contents);
+      return 0;
+    }
+
+    if (!BIO_reset(bio)) {
+      fprintf(stderr, "BIO_reset failed\n");
+      return 0;
+    }
+  }
+
+  BIO_free(bio);
+  return 1;
+}
+
 int main() {
   ERR_load_crypto_strings();
 
@@ -100,6 +151,10 @@ int main() {
     return 1;
   }
 
+  if (!test_printf()) {
+    return 1;
+  }
+
   printf("PASS\n");
   return 0;
 }
diff --git a/crypto/bio/printf.c b/crypto/bio/printf.c
index 7f7106f4..daebe74c 100644
--- a/crypto/bio/printf.c
+++ b/crypto/bio/printf.c
@@ -66,38 +66,30 @@
 
 #include <openssl/mem.h>
 
-
 int BIO_printf(BIO *bio, const char *format, ...) {
   va_list args;
-  int ret;
-
-  va_start(args, format);
-
-  ret = BIO_vprintf(bio, format, args);
-
-  va_end(args);
-  return ret;
-}
-
-int BIO_vprintf(BIO *bio, const char *format, va_list args) {
   char buf[256], *out, out_malloced = 0;
   int out_len, ret;
 
-  /* Note: this is assuming that va_list is ok to copy as POD. If the system
-   * defines it with a pointer to mutable state then passing it twice to
-   * vsnprintf will not work. */
+  va_start(args, format);
   out_len = vsnprintf(buf, sizeof(buf), format, args);
+  va_end(args);
+
   if (out_len >= sizeof(buf)) {
     const int requested_len = out_len;
-    /* The output was truncated. */
-    out = OPENSSL_malloc(requested_len);
+    /* The output was truncated. Note that vsnprintf's return value
+     * does not include a trailing NUL, but the buffer must be sized
+     * for it. */
+    out = OPENSSL_malloc(requested_len + 1);
     out_malloced = 1;
     if (out == NULL) {
       /* Unclear what can be done in this situation. OpenSSL has historically
        * crashed and that seems better than producing the wrong output. */
       abort();
     }
-    out_len = vsnprintf(out, requested_len, format, args);
+    va_start(args, format);
+    out_len = vsnprintf(out, requested_len + 1, format, args);
+    va_end(args);
     assert(out_len == requested_len);
   } else {
     out = buf;