commit 23223ebbc1c6a8400d126be0af2480880c70df3b
author David Benjamin <davidben@google.com> Thu Jan 25 18:02:45 2018 -0500
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Mon Feb 05 23:18:33 2018 +0000
tree 514ffd4949863fe2c86a21dc2396cfa3fc6fda5f
parent cb4e300f176429d091e07cbdb420748b7900f271

Tidy BN_bn2hex and BN_print with non-minimal inputs.

These actually work as-is, but BN_bn2hex allocates more memory than
necessary, and we may as well skip the unnecessary words where we can.
Also add a test for this.

Bug: 232
Change-Id: Ie271fe9f3901d00dd5c3d7d63c1776de81a10ec7
Reviewed-on: https://boringssl-review.googlesource.com/25304
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

crypto/bn_extra/convert.c

diff --git a/crypto/bn_extra/convert.c b/crypto/bn_extra/convert.c
index 6f3a062..b4a6014 100644
--- a/crypto/bn_extra/convert.c
+++ b/crypto/bn_extra/convert.c
@@ -77,8 +77,9 @@
 static const char hextable[] = "0123456789abcdef";
 
 char *BN_bn2hex(const BIGNUM *bn) {
+  int width = bn_minimal_width(bn);
   char *buf = OPENSSL_malloc(1 /* leading '-' */ + 1 /* zero is non-empty */ +
-                             bn->top * BN_BYTES * 2 + 1 /* trailing NUL */);
+                             width * BN_BYTES * 2 + 1 /* trailing NUL */);
   if (buf == NULL) {
     OPENSSL_PUT_ERROR(BN, ERR_R_MALLOC_FAILURE);
     return NULL;
@@ -94,7 +95,7 @@
   }
 
   int z = 0;
-  for (int i = bn->top - 1; i >= 0; i--) {
+  for (int i = width - 1; i >= 0; i--) {
     for (int j = BN_BITS2 - 8; j >= 0; j -= 8) {
       // strip leading zeros
       int v = ((int)(bn->d[i] >> (long)j)) & 0xff;
@@ -347,7 +348,7 @@
     goto end;
   }
 
-  for (i = a->top - 1; i >= 0; i--) {
+  for (i = bn_minimal_width(a) - 1; i >= 0; i--) {
     for (j = BN_BITS2 - 4; j >= 0; j -= 4) {
       // strip leading zeros
       v = ((int)(a->d[i] >> (long)j)) & 0x0f;

crypto/fipsmodule/bn/bn_test.cc

diff --git a/crypto/fipsmodule/bn/bn_test.cc b/crypto/fipsmodule/bn/bn_test.cc
index dcb55f8..757af27 100644
--- a/crypto/fipsmodule/bn/bn_test.cc
+++ b/crypto/fipsmodule/bn/bn_test.cc
@@ -87,6 +87,7 @@
 
 #include <gtest/gtest.h>
 
+#include <openssl/bio.h>
 #include <openssl/bn.h>
 #include <openssl/bytestring.h>
 #include <openssl/crypto.h>
@@ -1966,12 +1967,17 @@
   ASSERT_TRUE(two_exp_256);
   ASSERT_TRUE(BN_lshift(two_exp_256.get(), BN_value_one(), 256));
 
-  // Check some comparison functions on |ten| before and after expanding.
+  bssl::UniquePtr<BIGNUM> zero(BN_new());
+  ASSERT_TRUE(zero);
+  BN_zero(zero.get());
+
   for (size_t width = 1; width < 10; width++) {
     SCOPED_TRACE(width);
-    // Make a wider version of |ten|.
+    // Make |ten| and |zero| wider.
     EXPECT_TRUE(bn_resize_words(ten.get(), width));
     EXPECT_EQ(static_cast<int>(width), ten->top);
+    EXPECT_TRUE(bn_resize_words(zero.get(), width));
+    EXPECT_EQ(static_cast<int>(width), zero->top);
 
     EXPECT_TRUE(BN_abs_is_word(ten.get(), 10));
     EXPECT_TRUE(BN_is_word(ten.get(), 10));
@@ -2004,6 +2010,28 @@
     EXPECT_EQ(4u, BN_num_bits(ten.get()));
     EXPECT_EQ(1u, BN_num_bytes(ten.get()));
     EXPECT_FALSE(BN_is_pow2(ten.get()));
+
+    bssl::UniquePtr<char> hex(BN_bn2hex(ten.get()));
+    EXPECT_STREQ("0a", hex.get());
+    hex.reset(BN_bn2hex(zero.get()));
+    EXPECT_STREQ("0", hex.get());
+
+    bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem()));
+    ASSERT_TRUE(bio);
+    ASSERT_TRUE(BN_print(bio.get(), ten.get()));
+    const uint8_t *ptr;
+    size_t len;
+    ASSERT_TRUE(BIO_mem_contents(bio.get(), &ptr, &len));
+    // TODO(davidben): |BN_print| removes leading zeros within a byte, while
+    // |BN_bn2hex| rounds up to a byte, except for zero which it prints as
+    // "0". Fix this discrepancy?
+    EXPECT_EQ(Bytes("a"), Bytes(ptr, len));
+
+    bio.reset(BIO_new(BIO_s_mem()));
+    ASSERT_TRUE(bio);
+    ASSERT_TRUE(BN_print(bio.get(), zero.get()));
+    ASSERT_TRUE(BIO_mem_contents(bio.get(), &ptr, &len));
+    EXPECT_EQ(Bytes("0"), Bytes(ptr, len));
   }
 
   // |ten| may be resized back down to one word.