Browse code

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>

Alon Bar-Lev authored on 2012/04/01 22:46:28
Showing 5 changed files
... ...
@@ -769,6 +769,7 @@ show_pkcs11_ids (
769 769
 	const char * const provider,
770 770
 	bool cert_private
771 771
 ) {
772
+	struct gc_arena gc = gc_new();
772 773
 	pkcs11h_certificate_id_list_t user_certificates = NULL;
773 774
 	pkcs11h_certificate_id_list_t current = NULL;
774 775
 	CK_RV rv = CKR_FUNCTION_FAILED;
... ...
@@ -834,7 +835,6 @@ show_pkcs11_ids (
834 834
 	);
835 835
 	for (current = user_certificates;current != NULL; current = current->next) {
836 836
 		pkcs11h_certificate_t certificate = NULL;
837
-		struct gc_arena gc = gc_new();
838 837
 		char *dn = NULL;
839 838
 		char serial[1024] = {0};
840 839
 		char *ser = NULL;
... ...
@@ -927,8 +927,6 @@ show_pkcs11_ids (
927 927
 			free (ser);
928 928
 			ser = NULL;
929 929
 		}
930
-
931
-		gc_free (&gc);
932 930
 	}
933 931
 
934 932
 cleanup:
... ...
@@ -938,6 +936,7 @@ cleanup:
938 938
 	}
939 939
 
940 940
 	pkcs11h_terminate ();
941
+	gc_free (&gc);
941 942
 }
942 943
 
943 944
 #else
... ...
@@ -75,7 +75,7 @@ cleanup:
75 75
 char *
76 76
 pkcs11_certificate_dn (pkcs11h_certificate_t cert, struct gc_arena *gc)
77 77
 {
78
-  int ret = 1;
78
+  char *ret = NULL;
79 79
   char dn[1024] = {0};
80 80
 
81 81
   x509_cert polar_cert = {0};
... ...
@@ -90,14 +90,12 @@ pkcs11_certificate_dn (pkcs11h_certificate_t cert, struct gc_arena *gc)
90 90
       goto cleanup;
91 91
   }
92 92
 
93
-  ret = 0;
93
+  ret = string_alloc(dn, gc);
94 94
 
95 95
 cleanup:
96 96
   x509_free(&polar_cert);
97 97
 
98
-  if (ret == 0)
99
-    return string_alloc(dn, gc);
100
-  return NULL;
98
+  return ret;
101 99
 }
102 100
 
103 101
 int
... ...
@@ -538,8 +538,9 @@ verify_cert_call_command(const char *verify_command, struct env_set *es,
538 538
 static result_t
539 539
 verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
540 540
 {
541
+  result_t ret = FAILURE;
541 542
   char fn[256];
542
-  int fd;
543
+  int fd = -1;
543 544
   struct gc_arena gc = gc_new();
544 545
 
545 546
   char *serial = x509_get_serial(cert, &gc);
... ...
@@ -547,26 +548,29 @@ verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
547 547
   if (!openvpn_snprintf(fn, sizeof(fn), "%s%c%s", crl_dir, OS_SPECIFIC_DIRSEP, serial))
548 548
     {
549 549
       msg (D_HANDSHAKE, "VERIFY CRL: filename overflow");
550
-      gc_free(&gc);
551
-      return FAILURE;
550
+      goto cleanup;
552 551
     }
553 552
   fd = platform_open (fn, O_RDONLY, 0);
554 553
   if (fd >= 0)
555 554
     {
556 555
       msg (D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
557
-      close(fd);
558
-      gc_free(&gc);
559
-      return FAILURE;
556
+      goto cleanup;
560 557
     }
561 558
 
562
-  gc_free(&gc);
559
+  ret = SUCCESS;
563 560
 
564
-  return SUCCESS;
561
+cleanup:
562
+
563
+  if (fd != -1)
564
+    close(fd);
565
+  gc_free(&gc);
566
+  return ret;
565 567
 }
566 568
 
567 569
 result_t
568 570
 verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_depth)
569 571
 {
572
+  result_t ret = FAILURE;
570 573
   char *subject = NULL;
571 574
   char common_name[TLS_USERNAME_LEN] = {0};
572 575
   const struct tls_options *opt;
... ...
@@ -583,7 +587,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
583 583
     {
584 584
 	msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, could not extract X509 "
585 585
 	    "subject string from certificate", cert_depth);
586
-	goto err;
586
+	goto cleanup;
587 587
     }
588 588
 
589 589
   /* 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
602 602
 	       opt->x509_username_field,
603 603
 		 subject,
604 604
 		 TLS_USERNAME_LEN);
605
-	  goto err;
605
+	  goto cleanup;
606 606
 	}
607 607
     }
608 608
 
... ...
@@ -613,7 +617,7 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
613 613
   if (cert_depth >= MAX_CERT_DEPTH)
614 614
     {
615 615
       msg (D_TLS_ERRORS, "TLS Error: Convoluted certificate chain detected with depth [%d] greater than %d", cert_depth, MAX_CERT_DEPTH);
616
-      goto err;			/* Reject connection */
616
+      goto cleanup;			/* Reject connection */
617 617
     }
618 618
 
619 619
   /* 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
623 623
       if (memcmp (sha1_hash, opt->verify_hash, SHA_DIGEST_LENGTH))
624 624
       {
625 625
 	msg (D_TLS_ERRORS, "TLS Error: level-1 certificate hash verification failed");
626
-	goto err;
626
+	goto cleanup;
627 627
       }
628 628
     }
629 629
 
... ...
@@ -645,16 +649,16 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
645 645
 
646 646
   /* If this is the peer's own certificate, verify it */
647 647
   if (cert_depth == 0 && SUCCESS != verify_peer_cert(opt, cert, subject, common_name))
648
-    goto err;
648
+    goto cleanup;
649 649
 
650 650
   /* call --tls-verify plug-in(s), if registered */
651 651
   if (SUCCESS != verify_cert_call_plugin(opt->plugins, opt->es, cert_depth, cert, subject))
652
-    goto err;
652
+    goto cleanup;
653 653
 
654 654
   /* run --tls-verify script */
655 655
   if (opt->verify_command && SUCCESS != verify_cert_call_command(opt->verify_command,
656 656
       opt->es, cert_depth, cert, subject, opt->verify_export_cert))
657
-    goto err;
657
+    goto cleanup;
658 658
 
659 659
   /* check peer cert against CRL */
660 660
   if (opt->crl_file)
... ...
@@ -662,26 +666,29 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
662 662
       if (opt->ssl_flags & SSLF_CRL_VERIFY_DIR)
663 663
       {
664 664
 	if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert))
665
-	  goto err;
665
+	  goto cleanup;
666 666
       }
667 667
       else
668 668
       {
669 669
 	if (SUCCESS != x509_verify_crl(opt->crl_file, cert, subject))
670
-	  goto err;
670
+	  goto cleanup;
671 671
       }
672 672
     }
673 673
 
674 674
   msg (D_HANDSHAKE, "VERIFY OK: depth=%d, %s", cert_depth, subject);
675 675
   session->verified = true;
676
+  ret = SUCCESS;
676 677
 
677
-  gc_free(&gc);
678
-  return SUCCESS;
678
+cleanup:
679 679
 
680
- err:
681
-  tls_clear_error();
682
-  session->verified = false;
680
+  if (ret != SUCCESS)
681
+    {
682
+      tls_clear_error(); /* always? */
683
+      session->verified = false; /* double sure? */
684
+    }
683 685
   gc_free(&gc);
684
-  return FAILURE;
686
+
687
+  return ret;
685 688
 }
686 689
 
687 690
 /* ***************************************************************************
... ...
@@ -46,6 +46,7 @@
46 46
 int
47 47
 verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
48 48
 {
49
+  int ret = 0;
49 50
   struct tls_session *session;
50 51
   SSL *ssl;
51 52
   struct gc_arena gc = gc_new();
... ...
@@ -76,17 +77,19 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
76 76
 
77 77
       ERR_clear_error();
78 78
 
79
-      gc_free(&gc);
80 79
       session->verified = false;
81
-
82
-      return 0;
80
+      goto cleanup;
83 81
     }
84 82
 
83
+  if (SUCCESS != verify_cert(session, ctx->current_cert, ctx->error_depth))
84
+    goto cleanup;
85
+
86
+  ret = 1;
87
+
88
+cleanup:
85 89
   gc_free(&gc);
86 90
 
87
-  if (SUCCESS == verify_cert(session, ctx->current_cert, ctx->error_depth))
88
-    return 1;
89
-  return 0;
91
+  return ret;
90 92
 }
91 93
 
92 94
 #ifdef ENABLE_X509ALTUSERNAME
... ...
@@ -48,6 +48,7 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
48 48
 {
49 49
   struct tls_session *session = (struct tls_session *) session_obj;
50 50
   struct gc_arena gc = gc_new();
51
+  int ret = 1;
51 52
 
52 53
   ASSERT (cert);
53 54
   ASSERT (session);
... ...
@@ -68,18 +69,21 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,
68 68
 	msg (D_TLS_ERRORS, "VERIFY ERROR: depth=%d, could not extract X509 "
69 69
 	      "subject string from certificate", cert_depth);
70 70
 
71
-      gc_free(&gc);
72
-      return 1;
71
+      goto cleanup;
73 72
     }
74 73
 
74
+  if (SUCCESS != verify_cert(session, cert, cert_depth))
75
+    goto cleanup;
76
+
77
+  ret = 0;
78
+
79
+cleanup:
80
+  gc_free(&gc);
81
+
75 82
   /*
76 83
    * PolarSSL expects 1 on failure, 0 on success
77 84
    */
78
-  gc_free(&gc);
79
-
80
-  if (SUCCESS == verify_cert(session, cert, cert_depth))
81
-    return 0;
82
-  return 1;
85
+  return ret;
83 86
 }
84 87
 
85 88
 #ifdef ENABLE_X509ALTUSERNAME