[ruby/openssl] pkey: inline {rsa,dsa,dh,ec}_instance()

Merge the code into the callers so that the wrapping Ruby object is
allocated before the raw key object is allocated. This prevents possible
memory leak on Ruby object allocation failure, and also reduces the
lines of code.

https://github.com/ruby/openssl/commit/1eb1366615

4 files changed, 76 insertions, 156 deletions

diff --git a/ext/openssl/ossl_pkey_dh.c b/ext/openssl/ossl_pkey_dh.c
index dff69cfc33..bc50e5566b 100644
--- a/ext/openssl/ossl_pkey_dh.c
+++ b/ext/openssl/ossl_pkey_dh.c
@@ -30,31 +30,6 @@ VALUE cDH;
 VALUE eDHError;
 
 /*
- * Public
- */
-static VALUE
-dh_instance(VALUE klass, DH *dh)
-{
-    EVP_PKEY *pkey;
-    VALUE obj;
-
-    if (!dh) {
-	return Qfalse;
-    }
-    obj = NewPKey(klass);
-    if (!(pkey = EVP_PKEY_new())) {
-	return Qfalse;
-    }
-    if (!EVP_PKEY_assign_DH(pkey, dh)) {
-	EVP_PKEY_free(pkey);
-	return Qfalse;
-    }
-    SetPKey(obj, pkey);
-
-    return obj;
-}
-
-/*
  * Private
  */
 struct dh_blocking_gen_arg {
@@ -84,7 +59,7 @@ dh_generate(int size, int gen)
     if (!dh || !cb) {
 	DH_free(dh);
 	BN_GENCB_free(cb);
-	return NULL;
+        ossl_raise(eDHError, "malloc failure");
     }
 
     if (rb_block_given_p())
@@ -110,12 +85,12 @@ dh_generate(int size, int gen)
 	    ossl_clear_error();
 	    rb_jump_tag(cb_arg.state);
 	}
-	return NULL;
+        ossl_raise(eDHError, "DH_generate_parameters_ex");
     }
 
     if (!DH_generate_key(dh)) {
         DH_free(dh);
-        return NULL;
+        ossl_raise(eDHError, "DH_generate_key");
     }
 
     return dh;
@@ -136,6 +111,7 @@ dh_generate(int size, int gen)
 static VALUE
 ossl_dh_s_generate(int argc, VALUE *argv, VALUE klass)
 {
+    EVP_PKEY *pkey;
     DH *dh ;
     int g = 2;
     VALUE size, gen, obj;
@@ -143,13 +119,14 @@ ossl_dh_s_generate(int argc, VALUE *argv, VALUE klass)
     if (rb_scan_args(argc, argv, "11", &size, &gen) == 2) {
 	g = NUM2INT(gen);
     }
+    obj = rb_obj_alloc(klass);
+    GetPKey(obj, pkey);
+
     dh = dh_generate(NUM2INT(size), g);
-    obj = dh_instance(klass, dh);
-    if (obj == Qfalse) {
-	DH_free(dh);
-	ossl_raise(eDHError, NULL);
+    if (!EVP_PKEY_assign_DH(pkey, dh)) {
+        DH_free(dh);
+        ossl_raise(eDHError, "EVP_PKEY_assign_DH");
     }
-
     return obj;
 }
 
@@ -195,9 +172,7 @@ ossl_dh_initialize(int argc, VALUE *argv, VALUE self)
 	if (!NIL_P(gen)) {
 	    g = NUM2INT(gen);
 	}
-	if (!(dh = dh_generate(NUM2INT(arg), g))) {
-	    ossl_raise(eDHError, NULL);
-	}
+        dh = dh_generate(NUM2INT(arg), g);
     }
     else {
 	arg = ossl_to_der_if_possible(arg);
@@ -434,17 +409,21 @@ ossl_dh_to_text(VALUE self)
 static VALUE
 ossl_dh_to_public_key(VALUE self)
 {
+    EVP_PKEY *pkey;
     DH *orig_dh, *dh;
     VALUE obj;
 
+    obj = rb_obj_alloc(rb_obj_class(self));
+    GetPKey(obj, pkey);
+
     GetDH(self, orig_dh);
-    dh = DHparams_dup(orig_dh); /* err check perfomed by dh_instance */
-    obj = dh_instance(rb_obj_class(self), dh);
-    if (obj == Qfalse) {
-	DH_free(dh);
-	ossl_raise(eDHError, NULL);
+    dh = DHparams_dup(orig_dh);
+    if (!dh)
+        ossl_raise(eDHError, "DHparams_dup");
+    if (!EVP_PKEY_assign_DH(pkey, dh)) {
+        DH_free(dh);
+        ossl_raise(eDHError, "EVP_PKEY_assign_DH");
     }
-
     return obj;
 }
 
diff --git a/ext/openssl/ossl_pkey_dsa.c b/ext/openssl/ossl_pkey_dsa.c
index e9be9ac482..c907f31c19 100644
--- a/ext/openssl/ossl_pkey_dsa.c
+++ b/ext/openssl/ossl_pkey_dsa.c
@@ -44,31 +44,6 @@ VALUE cDSA;
 VALUE eDSAError;
 
 /*
- * Public
- */
-static VALUE
-dsa_instance(VALUE klass, DSA *dsa)
-{
-    EVP_PKEY *pkey;
-    VALUE obj;
-
-    if (!dsa) {
-	return Qfalse;
-    }
-    obj = NewPKey(klass);
-    if (!(pkey = EVP_PKEY_new())) {
-	return Qfalse;
-    }
-    if (!EVP_PKEY_assign_DSA(pkey, dsa)) {
-	EVP_PKEY_free(pkey);
-	return Qfalse;
-    }
-    SetPKey(obj, pkey);
-
-    return obj;
-}
-
-/*
  * Private
  */
 struct dsa_blocking_gen_arg {
@@ -100,9 +75,9 @@ dsa_generate(int size)
     unsigned long h;
 
     if (!dsa || !cb) {
-	DSA_free(dsa);
-	BN_GENCB_free(cb);
-	return NULL;
+        DSA_free(dsa);
+        BN_GENCB_free(cb);
+        ossl_raise(eDSAError, "malloc failure");
     }
 
     if (rb_block_given_p())
@@ -132,12 +107,12 @@ dsa_generate(int size)
 	    ossl_clear_error();
 	    rb_jump_tag(cb_arg.state);
 	}
-	return NULL;
+        ossl_raise(eDSAError, "DSA_generate_parameters_ex");
     }
 
     if (!DSA_generate_key(dsa)) {
-	DSA_free(dsa);
-	return NULL;
+        DSA_free(dsa);
+        ossl_raise(eDSAError, "DSA_generate_key");
     }
 
     return dsa;
@@ -157,14 +132,18 @@ dsa_generate(int size)
 static VALUE
 ossl_dsa_s_generate(VALUE klass, VALUE size)
 {
-    DSA *dsa = dsa_generate(NUM2INT(size)); /* err handled by dsa_instance */
-    VALUE obj = dsa_instance(klass, dsa);
+    EVP_PKEY *pkey;
+    DSA *dsa;
+    VALUE obj;
 
-    if (obj == Qfalse) {
-	DSA_free(dsa);
-	ossl_raise(eDSAError, NULL);
-    }
+    obj = rb_obj_alloc(klass);
+    GetPKey(obj, pkey);
 
+    dsa = dsa_generate(NUM2INT(size));
+    if (!EVP_PKEY_assign_DSA(pkey, dsa)) {
+        DSA_free(dsa);
+        ossl_raise(eDSAError, "EVP_PKEY_assign_DSA");
+    }
     return obj;
 }
 
@@ -460,20 +439,23 @@ ossl_dsa_to_text(VALUE self)
 static VALUE
 ossl_dsa_to_public_key(VALUE self)
 {
-    EVP_PKEY *pkey;
+    EVP_PKEY *pkey, *pkey_new;
     DSA *dsa;
     VALUE obj;
 
     GetPKeyDSA(self, pkey);
-    /* err check performed by dsa_instance */
+    obj = rb_obj_alloc(rb_obj_class(self));
+    GetPKey(obj, pkey_new);
+
 #define DSAPublicKey_dup(dsa) (DSA *)ASN1_dup( \
 	(i2d_of_void *)i2d_DSAPublicKey, (d2i_of_void *)d2i_DSAPublicKey, (char *)(dsa))
     dsa = DSAPublicKey_dup(EVP_PKEY_get0_DSA(pkey));
 #undef DSAPublicKey_dup
-    obj = dsa_instance(rb_obj_class(self), dsa);
-    if (obj == Qfalse) {
-	DSA_free(dsa);
-	ossl_raise(eDSAError, NULL);
+    if (!dsa)
+        ossl_raise(eDSAError, "DSAPublicKey_dup");
+    if (!EVP_PKEY_assign_DSA(pkey_new, dsa)) {
+        DSA_free(dsa);
+        ossl_raise(eDSAError, "EVP_PKEY_assign_DSA");
     }
     return obj;
 }
diff --git a/ext/openssl/ossl_pkey_ec.c b/ext/openssl/ossl_pkey_ec.c
index eabf495f19..aec9d1e60f 100644
--- a/ext/openssl/ossl_pkey_ec.c
+++ b/ext/openssl/ossl_pkey_ec.c
@@ -63,27 +63,6 @@ static ID id_i_group;
 static VALUE ec_group_new(const EC_GROUP *group);
 static VALUE ec_point_new(const EC_POINT *point, const EC_GROUP *group);
 
-static VALUE ec_instance(VALUE klass, EC_KEY *ec)
-{
-    EVP_PKEY *pkey;
-    VALUE obj;
-
-    if (!ec) {
-	return Qfalse;
-    }
-    obj = NewPKey(klass);
-    if (!(pkey = EVP_PKEY_new())) {
-	return Qfalse;
-    }
-    if (!EVP_PKEY_assign_EC_KEY(pkey, ec)) {
-	EVP_PKEY_free(pkey);
-	return Qfalse;
-    }
-    SetPKey(obj, pkey);
-
-    return obj;
-}
-
 /*
  * Creates a new EC_KEY on the EC group obj. arg can be an EC::Group or a String
  * representing an OID.
@@ -130,17 +109,18 @@ ec_key_new_from_group(VALUE arg)
 static VALUE
 ossl_ec_key_s_generate(VALUE klass, VALUE arg)
 {
+    EVP_PKEY *pkey;
     EC_KEY *ec;
     VALUE obj;
 
-    ec = ec_key_new_from_group(arg);
+    obj = rb_obj_alloc(klass);
+    GetPKey(obj, pkey);
 
-    obj = ec_instance(klass, ec);
-    if (obj == Qfalse) {
-	EC_KEY_free(ec);
-	ossl_raise(eECError, NULL);
+    ec = ec_key_new_from_group(arg);
+    if (!EVP_PKEY_assign_EC_KEY(pkey, ec)) {
+        EC_KEY_free(ec);
+        ossl_raise(eECError, "EVP_PKEY_assign_EC_KEY");
     }
-
     if (!EC_KEY_generate_key(ec))
 	ossl_raise(eECError, "EC_KEY_generate_key");
 
diff --git a/ext/openssl/ossl_pkey_rsa.c b/ext/openssl/ossl_pkey_rsa.c
index c1ae44fe40..fbdb9c8960 100644
--- a/ext/openssl/ossl_pkey_rsa.c
+++ b/ext/openssl/ossl_pkey_rsa.c
@@ -45,31 +45,6 @@ VALUE cRSA;
 VALUE eRSAError;
 
 /*
- * Public
- */
-static VALUE
-rsa_instance(VALUE klass, RSA *rsa)
-{
-    EVP_PKEY *pkey;
-    VALUE obj;
-
-    if (!rsa) {
-	return Qfalse;
-    }
-    obj = NewPKey(klass);
-    if (!(pkey = EVP_PKEY_new())) {
-	return Qfalse;
-    }
-    if (!EVP_PKEY_assign_RSA(pkey, rsa)) {
-	EVP_PKEY_free(pkey);
-	return Qfalse;
-    }
-    SetPKey(obj, pkey);
-
-    return obj;
-}
-
-/*
  * Private
  */
 struct rsa_blocking_gen_arg {
@@ -102,7 +77,7 @@ rsa_generate(int size, unsigned long exp)
 	RSA_free(rsa);
 	BN_free(e);
 	BN_GENCB_free(cb);
-	return NULL;
+        ossl_raise(eRSAError, "malloc failure");
     }
     for (i = 0; i < (int)sizeof(exp) * 8; ++i) {
 	if (exp & (1UL << i)) {
@@ -110,7 +85,7 @@ rsa_generate(int size, unsigned long exp)
 		BN_free(e);
 		RSA_free(rsa);
 		BN_GENCB_free(cb);
-		return NULL;
+                ossl_raise(eRSAError, "BN_set_bit");
 	    }
 	}
     }
@@ -139,7 +114,7 @@ rsa_generate(int size, unsigned long exp)
 	    ossl_clear_error();
 	    rb_jump_tag(cb_arg.state);
 	}
-	return NULL;
+        ossl_raise(eRSAError, "RSA_generate_key_ex");
     }
 
     return rsa;
@@ -158,26 +133,26 @@ static VALUE
 ossl_rsa_s_generate(int argc, VALUE *argv, VALUE klass)
 {
 /* why does this method exist?  why can't initialize take an optional exponent? */
+    EVP_PKEY *pkey;
     RSA *rsa;
     VALUE size, exp;
     VALUE obj;
 
     rb_scan_args(argc, argv, "11", &size, &exp);
+    obj = rb_obj_alloc(klass);
+    GetPKey(obj, pkey);
 
-    rsa = rsa_generate(NUM2INT(size), NIL_P(exp) ? RSA_F4 : NUM2ULONG(exp)); /* err handled by rsa_instance */
-    obj = rsa_instance(klass, rsa);
-
-    if (obj == Qfalse) {
-	RSA_free(rsa);
-	ossl_raise(eRSAError, NULL);
+    rsa = rsa_generate(NUM2INT(size), NIL_P(exp) ? RSA_F4 : NUM2ULONG(exp));
+    if (!EVP_PKEY_assign_RSA(pkey, rsa)) {
+        RSA_free(rsa);
+        ossl_raise(eRSAError, "EVP_PKEY_assign_RSA");
     }
-
     return obj;
 }
 
 /*
  * call-seq:
- *   RSA.new(key_size)                 => RSA instance
+ *   RSA.new(size [, exponent])        => RSA instance
  *   RSA.new(encoded_key)              => RSA instance
  *   RSA.new(encoded_key, pass_phrase) => RSA instance
  *
@@ -206,10 +181,11 @@ ossl_rsa_initialize(int argc, VALUE *argv, VALUE self)
     GetPKey(self, pkey);
     if(rb_scan_args(argc, argv, "02", &arg, &pass) == 0) {
 	rsa = RSA_new();
+        if (!rsa)
+            ossl_raise(eRSAError, "RSA_new");
     }
     else if (RB_INTEGER_TYPE_P(arg)) {
 	rsa = rsa_generate(NUM2INT(arg), NIL_P(pass) ? RSA_F4 : NUM2ULONG(pass));
-	if (!rsa) ossl_raise(eRSAError, NULL);
     }
     else {
 	pass = ossl_pem_passwd_value(pass);
@@ -243,7 +219,7 @@ ossl_rsa_initialize(int argc, VALUE *argv, VALUE self)
     }
     if (!EVP_PKEY_assign_RSA(pkey, rsa)) {
 	RSA_free(rsa);
-	ossl_raise(eRSAError, NULL);
+	ossl_raise(eRSAError, "EVP_PKEY_assign_RSA");
     }
 
     return self;
@@ -787,17 +763,20 @@ ossl_rsa_to_text(VALUE self)
 static VALUE
 ossl_rsa_to_public_key(VALUE self)
 {
-    EVP_PKEY *pkey;
+    EVP_PKEY *pkey, *pkey_new;
     RSA *rsa;
     VALUE obj;
 
     GetPKeyRSA(self, pkey);
-    /* err check performed by rsa_instance */
+    obj = rb_obj_alloc(rb_obj_class(self));
+    GetPKey(obj, pkey_new);
+
     rsa = RSAPublicKey_dup(EVP_PKEY_get0_RSA(pkey));
-    obj = rsa_instance(rb_obj_class(self), rsa);
-    if (obj == Qfalse) {
-	RSA_free(rsa);
-	ossl_raise(eRSAError, NULL);
+    if (!rsa)
+        ossl_raise(eRSAError, "RSAPublicKey_dup");
+    if (!EVP_PKEY_assign_RSA(pkey_new, rsa)) {
+        RSA_free(rsa);
+        ossl_raise(eRSAError, "EVP_PKEY_assign_RSA");
     }
     return obj;
 }