commit 8462a367bb57e9524c3d8eca9c62733c63a63cf4
author David Benjamin <davidben@google.com> Thu Sep 01 16:02:22 2022 -0400
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Sep 01 22:01:19 2022 +0000
tree c04f9cfe48ea36ffa3d31970de3468a3947c0352
parent 2749466282eaf639e6acd581a92e827c6ecbd622

Silence a GCC 12 -Warray-bounds false positive warning.

GCC 12 triggers a -Warray-bounds false positive in crypto/x509v3's IPv6
parser. Although v6stat.total cannot exceed 16 because of the callback,
GCC doesn't know this and seems to get confused. Checking >= 16 seems to
silence it.

While I'm here, move the comments so they don't obscure the
if/else-if chains and avoid a theoretical overflow in 'zero_cnt' by
checking for the maximum value inside the callback.

Change-Id: If1610a36693915aa92085d8cb3a4709ae82992ba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54245
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/x509v3/v3_utl.c

diff --git a/crypto/x509v3/v3_utl.c b/crypto/x509v3/v3_utl.c
index 15ebe54..76df91c 100644
--- a/crypto/x509v3/v3_utl.c
+++ b/crypto/x509v3/v3_utl.c
@@ -1233,8 +1233,6 @@
     return 0;
   }
 
-  // Now for some sanity checks
-
   if (v6stat.zero_pos == -1) {
     // If no '::' must have exactly 16 bytes
     if (v6stat.total != 16) {
@@ -1242,35 +1240,31 @@
     }
   } else {
     // If '::' must have less than 16 bytes
-    if (v6stat.total == 16) {
+    if (v6stat.total >= 16) {
       return 0;
     }
-    // More than three zeroes is an error
     if (v6stat.zero_cnt > 3) {
+      // More than three zeroes is an error
       return 0;
-    }
-    // Can only have three zeroes if nothing else present
-    else if (v6stat.zero_cnt == 3) {
+    } else if (v6stat.zero_cnt == 3) {
+      // Can only have three zeroes if nothing else present
       if (v6stat.total > 0) {
         return 0;
       }
-    }
-    // Can only have two zeroes if at start or end
-    else if (v6stat.zero_cnt == 2) {
-      if ((v6stat.zero_pos != 0) && (v6stat.zero_pos != v6stat.total)) {
+    } else if (v6stat.zero_cnt == 2) {
+      // Can only have two zeroes if at start or end
+      if (v6stat.zero_pos != 0 && v6stat.zero_pos != v6stat.total) {
         return 0;
       }
-    } else
-    // Can only have one zero if *not* start or end
-    {
-      if ((v6stat.zero_pos == 0) || (v6stat.zero_pos == v6stat.total)) {
+    } else {
+      // Can only have one zero if *not* start or end
+      if (v6stat.zero_pos == 0 || v6stat.zero_pos == v6stat.total) {
         return 0;
       }
     }
   }
 
-  // Format result
-
+  // Format the result.
   if (v6stat.zero_pos >= 0) {
     // Copy initial part
     OPENSSL_memcpy(v6, v6stat.tmp, v6stat.zero_pos);
@@ -1299,9 +1293,12 @@
     // Zero length element, corresponds to '::'
     if (s->zero_pos == -1) {
       s->zero_pos = s->total;
+    } else if (s->zero_pos != s->total) {
+      // If we've already got a :: its an error
+      return 0;
     }
-    // If we've already got a :: its an error
-    else if (s->zero_pos != s->total) {
+    if (s->zero_cnt >= 3) {
+      // More than three zeros is an error.
       return 0;
     }
     s->zero_cnt++;