cleanup: gc usage

Cleanup of "Use the garbage collector when retrieving x509 fields"
patch series.

Discussed at [1].

There should be an effort to produce common function prologue
and epilogue, so that cleanups will be done at single point.

[1] http://comments.gmane.org/gmane.network.openvpn.devel/5401

Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
Acked-by: Adriaan de Jong <dejong@fox-it.com>
Signed-off-by: David Sommerseth <davids@redhat.com>

diff --git a/src/openvpn/pkcs11.c b/src/openvpn/pkcs11.c
index a6b8db5020874e9d0c78275eb27102c284aac079..d86e267ccae18b77c5c0007b602ea1735902d894 100644 (file)
--- a/src/openvpn/pkcs11.c
+++ b/src/openvpn/pkcs11.c
@@ -769,6 +769,7 @@ show_pkcs11_ids (
        const char * const provider,
        bool cert_private
 ) {
+       struct gc_arena gc = gc_new();
        pkcs11h_certificate_id_list_t user_certificates = NULL;
        pkcs11h_certificate_id_list_t current = NULL;
        CK_RV rv = CKR_FUNCTION_FAILED;
@@ -834,7 +835,6 @@ show_pkcs11_ids (
        );
        for (current = user_certificates;current != NULL; current = current->next) {
                pkcs11h_certificate_t certificate = NULL;
-               struct gc_arena gc = gc_new();
                char *dn = NULL;
                char serial[1024] = {0};
                char *ser = NULL;
@@ -927,8 +927,6 @@ show_pkcs11_ids (
                        free (ser);
                        ser = NULL;
                }
-
-               gc_free (&gc);
        }
 
 cleanup:
@@ -938,6 +936,7 @@ cleanup:
        }
 
        pkcs11h_terminate ();
+       gc_free (&gc);
 }
 
 #else

diff --git a/src/openvpn/pkcs11_polarssl.c b/src/openvpn/pkcs11_polarssl.c
index f5b7b8b388efcf75d48fc7d4b604b48e1203b916..03b2bab5be8bad62deb72350adefb3a36a929339 100644 (file)
--- a/src/openvpn/pkcs11_polarssl.c
+++ b/src/openvpn/pkcs11_polarssl.c
@@ -75,7 +75,7 @@ cleanup:
 char *
 pkcs11_certificate_dn (pkcs11h_certificate_t cert, struct gc_arena *gc)
 {
-  int ret = 1;
+  char *ret = NULL;
   char dn[1024] = {0};
 
   x509_cert polar_cert = {0};
@@ -90,14 +90,12 @@ pkcs11_certificate_dn (pkcs11h_certificate_t cert, struct gc_arena *gc)
       goto cleanup;
   }
 
-  ret = 0;
+  ret = string_alloc(dn, gc);
 
 cleanup:
   x509_free(&polar_cert);
 
-  if (ret == 0)
-    return string_alloc(dn, gc);
-  return NULL;
+  return ret;
 }
 
 int

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 578352802c2cc5c6f4774995e30eafa5b5970f34..30fb05dfa2c2a3454c9353fb5fc3b0b8e33089f4 100644 (file)
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -538,8 +538,9 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
 static result_t
 verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
 {
+  result_t ret = FAILURE;
   char fn[256];
-  int fd;
+  int fd = -1;
   struct gc_arena gc = gc_new();
 
   char *serial = x509_get_serial(cert, &gc);
@@ -547,26 +548,29 @@ verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
   if (!openvpn_snprintf(fn, sizeof(fn), "%s%c%s", crl_dir, OS_SPECIFIC_DIRSEP, serial))
     {
       msg (D_HANDSHAKE, "VERIFY CRL: filename overflow");
-      gc_free(&gc);
-      return FAILURE;
+      goto cleanup;
     }
   fd = platform_open (fn, O_RDONLY, 0);
   if (fd >= 0)
     {
       msg (D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
-      close(fd);
-      gc_free(&gc);
-      return FAILURE;
+      goto cleanup;
     }
 
-  gc_free(&gc);
+  ret = SUCCESS;
 
-  return SUCCESS;
+cleanup:
+
+  if (fd != -1)
+    close(fd);
+  gc_free(&gc);
+  return ret;
 }
 
 result_t
 verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_depth)
 {
+  result_t ret = FAILURE;
   char *subject = NULL;
   char common_name[TLS_USERNAME_LEN] = {0};
   const struct tls_options *opt;
@@ -583,7 +587,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
     {
        msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, could not extract X509 "
            "subject string from certificate", cert_depth);
-       goto err;
+       goto cleanup;
     }
 
   /* enforce character class restrictions in X509 name */
@@ -602,7 +606,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
               opt->x509_username_field,
                 subject,
                 TLS_USERNAME_LEN);
-         goto err;
+         goto cleanup;
        }
     }
 
@@ -613,7 +617,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
   if (cert_depth >= MAX_CERT_DEPTH)
     {
       msg (D_TLS_ERRORS, "TLS Error: Convoluted certificate chain detected with depth [%d] greater than %d", cert_depth, MAX_CERT_DEPTH);
-      goto err;                        /* Reject connection */
+      goto cleanup;                    /* Reject connection */
     }
 
   /* verify level 1 cert, i.e. the CA that signed our leaf cert */
@@ -623,7 +627,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
       if (memcmp (sha1_hash, opt->verify_hash, SHA_DIGEST_LENGTH))
       {
        msg (D_TLS_ERRORS, "TLS Error: level-1 certificate hash verification failed");
-       goto err;
+       goto cleanup;
       }
     }
 
@@ -645,16 +649,16 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
 
   /* If this is the peer's own certificate, verify it */
   if (cert_depth == 0 && SUCCESS != verify_peer_cert(opt, cert, subject, common_name))
-    goto err;
+    goto cleanup;
 
   /* call --tls-verify plug-in(s), if registered */
   if (SUCCESS != verify_cert_call_plugin(opt->plugins, opt->es, cert_depth, cert, subject))
-    goto err;
+    goto cleanup;
 
   /* run --tls-verify script */
   if (opt->verify_command && SUCCESS != verify_cert_call_command(opt->verify_command,
       opt->es, cert_depth, cert, subject, opt->verify_export_cert))
-    goto err;
+    goto cleanup;
 
   /* check peer cert against CRL */
   if (opt->crl_file)
@@ -662,26 +666,29 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
       if (opt->ssl_flags & SSLF_CRL_VERIFY_DIR)
       {
        if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert))
-         goto err;
+         goto cleanup;
       }
       else
       {
        if (SUCCESS != x509_verify_crl(opt->crl_file, cert, subject))
-         goto err;
+         goto cleanup;
       }
     }
 
   msg (D_HANDSHAKE, "VERIFY OK: depth=%d, %s", cert_depth, subject);
   session->verified = true;
+  ret = SUCCESS;
 
-  gc_free(&gc);
-  return SUCCESS;
+cleanup:
 
- err:
-  tls_clear_error();
-  session->verified = false;
+  if (ret != SUCCESS)
+    {
+      tls_clear_error(); /* always? */
+      session->verified = false; /* double sure? */
+    }
   gc_free(&gc);
-  return FAILURE;
+
+  return ret;
 }
 
 /* ***************************************************************************

diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 4dfabfc014f66d15db2a4312bd012a2db37b9a20..f5dce0dc98535c2bc986d15815527639a13469c0 100644 (file)
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -46,6 +46,7 @@
 int
 verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
 {
+  int ret = 0;
   struct tls_session *session;
   SSL *ssl;
   struct gc_arena gc = gc_new();
@@ -76,17 +77,19 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
 
       ERR_clear_error();
 
-      gc_free(&gc);
       session->verified = false;
-
-      return 0;
+      goto cleanup;
     }
 
+  if (SUCCESS != verify_cert(session, ctx->current_cert, ctx->error_depth))
+    goto cleanup;
+
+  ret = 1;
+
+cleanup:
   gc_free(&gc);
 
-  if (SUCCESS == verify_cert(session, ctx->current_cert, ctx->error_depth))
-    return 1;
-  return 0;
+  return ret;
 }
 
 #ifdef ENABLE_X509ALTUSERNAME

diff --git a/src/openvpn/ssl_verify_polarssl.c b/src/openvpn/ssl_verify_polarssl.c
index d9d4fd584be7504bb3df01a52a8e0df22af581dd..a32db8df1b9364400ae7003c1649cb8b6b230e2d 100644 (file)
--- a/src/openvpn/ssl_verify_polarssl.c
+++ b/src/openvpn/ssl_verify_polarssl.c
@@ -48,6 +48,7 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
 {
   struct tls_session *session = (struct tls_session *) session_obj;
   struct gc_arena gc = gc_new();
+  int ret = 1;
 
   ASSERT (cert);
   ASSERT (session);
@@ -68,18 +69,21 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
        msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, could not extract X509 "
              "subject string from certificate", cert_depth);
 
-      gc_free(&gc);
-      return 1;
+      goto cleanup;
     }
 
+  if (SUCCESS != verify_cert(session, cert, cert_depth))
+    goto cleanup;
+
+  ret = 0;
+
+cleanup:
+  gc_free(&gc);
+
   /*
    * PolarSSL expects 1 on failure, 0 on success
    */
-  gc_free(&gc);
-
-  if (SUCCESS == verify_cert(session, cert, cert_depth))
-    return 0;
-  return 1;
+  return ret;
 }
 
 #ifdef ENABLE_X509ALTUSERNAME