Turn the validation_err() macro into a function

validation_err() is an ugly macro with side effects and a goto in it.
At the cost of a few lines of code we can turn this into a function
where the side effects are explicit and ret is now explicitly set in
the main body of addr_validate_path_internal().

We get to a point where it is halfway possible to reason about the
convoluted control flow in this function.

ok inoguchi jsing

1 files changed, 44 insertions, 31 deletions

diff --git a/src/lib/libcrypto/x509/x509_addr.c b/src/lib/libcrypto/x509/x509_addr.c
index bee852d8db..dac9d8e055 100644
--- a/src/lib/libcrypto/x509/x509_addr.c
+++ b/src/lib/libcrypto/x509/x509_addr.c
@@ -1,4 +1,4 @@
-/*      $OpenBSD: x509_addr.c,v 1.63 2022/01/05 17:27:40 tb Exp $ */
+/*      $OpenBSD: x509_addr.c,v 1.64 2022/01/05 17:36:32 tb Exp $ */
 /*
  * Contributed to the OpenSSL Project by the American Registry for
  * Internet Numbers ("ARIN").
@@ -1719,22 +1719,18 @@ X509v3_addr_subset(IPAddrBlocks *child, IPAddrBlocks *parent)
         return 1;
 }
 
-/*
- * Validation error handling via callback.
- */
-#define validation_err(_err_)           \
-  do {                                  \
-    if (ctx != NULL) {                  \
-      ctx->error = _err_;               \
-      ctx->error_depth = i;             \
-      ctx->current_cert = x;            \
-      ret = ctx->verify_cb(0, ctx);     \
-    } else {                            \
-      ret = 0;                          \
-    }                                   \
-    if (!ret)                           \
-      goto done;                        \
-  } while (0)
+static int
+verify_error(X509_STORE_CTX *ctx, X509 *cert, int error, int depth)
+{
+        if (ctx == NULL)
+                return 0;
+
+        ctx->current_cert = cert;
+        ctx->error = error;
+        ctx->error_depth = depth;
+
+        return ctx->verify_cb(0, ctx);
+}
 
 /*
  * Core code for RFC 3779 2.3 path validation.
@@ -1780,8 +1776,13 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain,
                 if ((ext = x->rfc3779_addr) == NULL)
                         goto done;
         }
-        if (!X509v3_addr_is_canonical(ext))
-                validation_err(X509_V_ERR_INVALID_EXTENSION);
+
+        if (!X509v3_addr_is_canonical(ext)) {
+                if ((ret = verify_error(ctx, x,
+                    X509_V_ERR_INVALID_EXTENSION, i)) == 0)
+                        goto done;
+        }
+
         (void)sk_IPAddressFamily_set_cmp_func(ext, IPAddressFamily_cmp);
         if ((child = sk_IPAddressFamily_dup(ext)) == NULL) {
                 X509V3error(ERR_R_MALLOC_FAILURE);
@@ -1802,16 +1803,22 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain,
                         for (j = 0; j < sk_IPAddressFamily_num(child); j++) {
                                 fc = sk_IPAddressFamily_value(child, j);
 
-                                if (IPAddressFamily_inheritance(fc) == NULL) {
-                                        validation_err(X509_V_ERR_UNNESTED_RESOURCE);
-                                        break;
-                                }
+                                if (IPAddressFamily_inheritance(fc) != NULL)
+                                        continue;
+
+                                if ((ret = verify_error(ctx, x,
+                                    X509_V_ERR_UNNESTED_RESOURCE, i)) == 0)
+                                        goto done;
+                                break;
                         }
                         continue;
                 }
 
-                if (!X509v3_addr_is_canonical(parent))
-                        validation_err(X509_V_ERR_INVALID_EXTENSION);
+                if (!X509v3_addr_is_canonical(parent)) {
+                        if ((ret = verify_error(ctx, x,
+                            X509_V_ERR_INVALID_EXTENSION, i)) == 0)
+                                goto done;
+                }
 
                 sk_IPAddressFamily_set_cmp_func(parent, IPAddressFamily_cmp);
 
@@ -1836,7 +1843,9 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain,
                                         continue;
 
                                 /* Otherwise the child isn't covered. */
-                                validation_err(X509_V_ERR_UNNESTED_RESOURCE);
+                                if ((ret = verify_error(ctx, x,
+                                    X509_V_ERR_UNNESTED_RESOURCE, i)) == 0)
+                                        goto done;
                                 break;
                         }
 
@@ -1870,7 +1879,9 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain,
                                 continue;
                         }
 
-                        validation_err(X509_V_ERR_UNNESTED_RESOURCE);
+                        if ((ret = verify_error(ctx, x,
+                            X509_V_ERR_UNNESTED_RESOURCE, i)) == 0)
+                                goto done;
                 }
         }
 
@@ -1884,8 +1895,12 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain,
                         if (IPAddressFamily_inheritance(fp) == NULL)
                                 continue;
 
-                        if (sk_IPAddressFamily_find(child, fp) >= 0)
-                                validation_err(X509_V_ERR_UNNESTED_RESOURCE);
+                        if (sk_IPAddressFamily_find(child, fp) < 0)
+                                continue;
+
+                        if ((ret = verify_error(ctx, x,
+                            X509_V_ERR_UNNESTED_RESOURCE, i)) == 0)
+                                goto done;
                 }
         }
 
@@ -1902,8 +1917,6 @@ addr_validate_path_internal(X509_STORE_CTX *ctx, STACK_OF(X509) *chain,
         return 0;
 }
 
-#undef validation_err
-
 /*
  * RFC 3779 2.3 path validation -- called from X509_verify_cert().
  */