Revamp get_dss_private_key()

* Simplify logic by having incremental allocation and only free on error
  in one place.
* Add error checking on all OpenSSL calls.
* Make it explicit when you need to be careful with non-ref counted pointers.
  - set0 calls will save the pointer without reference counting.
  - On success, set pointers to NULL to avoid double frees since the struct
    is now responsible for freeing the resources.

1 files changed, 51 insertions, 20 deletions

diff --git a/lib/crypto/c_src/dss.c b/lib/crypto/c_src/dss.c
index 9d39241382..934b33d87c 100644
--- a/lib/crypto/c_src/dss.c
+++ b/lib/crypto/c_src/dss.c
@@ -26,36 +26,67 @@ int get_dss_private_key(ErlNifEnv* env, ERL_NIF_TERM key, DSA *dsa)
     /* key=[P,Q,G,KEY] */
     ERL_NIF_TERM head, tail;
     BIGNUM *dsa_p = NULL, *dsa_q = NULL, *dsa_g = NULL;
-    BIGNUM *dummy_pub_key, *priv_key = NULL;
+    BIGNUM *dummy_pub_key = NULL, *priv_key = NULL;
 
-    if (!enif_get_list_cell(env, key, &head, &tail)
-	|| !get_bn_from_bin(env, head, &dsa_p)
-	|| !enif_get_list_cell(env, tail, &head, &tail)
-	|| !get_bn_from_bin(env, head, &dsa_q)
-	|| !enif_get_list_cell(env, tail, &head, &tail)
-	|| !get_bn_from_bin(env, head, &dsa_g)
-	|| !enif_get_list_cell(env, tail, &head, &tail)
-	|| !get_bn_from_bin(env, head, &priv_key)
-	|| !enif_is_empty_list(env,tail)) {
-	if (dsa_p) BN_free(dsa_p);
-	if (dsa_q) BN_free(dsa_q);
-	if (dsa_g) BN_free(dsa_g);
-	if (priv_key) BN_free(priv_key);
-	return 0;
-    }
+    if (!enif_get_list_cell(env, key, &head, &tail))
+        goto err;
+    if (!get_bn_from_bin(env, head, &dsa_p))
+        goto err;
+
+    if (!enif_get_list_cell(env, tail, &head, &tail))
+        goto err;
+    if (!get_bn_from_bin(env, head, &dsa_q))
+        goto err;
+
+    if (!enif_get_list_cell(env, tail, &head, &tail))
+        goto err;
+    if (!get_bn_from_bin(env, head, &dsa_g))
+        goto err;
+
+    if (!enif_get_list_cell(env, tail, &head, &tail))
+        goto err;
+    if (!get_bn_from_bin(env, head, &priv_key))
+        goto err;
+
+    if (!enif_is_empty_list(env, tail))
+        goto err;
 
     /* Note: DSA_set0_key() does not allow setting only the
      * private key, although DSA_sign() does not use the
      * public key. Work around this limitation by setting
      * the public key to a copy of the private key.
      */
-    dummy_pub_key = BN_dup(priv_key);
+    if ((dummy_pub_key = BN_dup(priv_key)) == NULL)
+        goto err;
+
+    if (!DSA_set0_pqg(dsa, dsa_p, dsa_q, dsa_g))
+        goto err;
+    /* dsa takes ownership on success */
+    dsa_p = NULL;
+    dsa_q = NULL;
+    dsa_g = NULL;
+
+    if (!DSA_set0_key(dsa, dummy_pub_key, priv_key))
+        goto err;
+    /* dsa takes ownership on success */
+    dummy_pub_key = NULL;
+    priv_key = NULL;
 
-    DSA_set0_pqg(dsa, dsa_p, dsa_q, dsa_g);
-    DSA_set0_key(dsa, dummy_pub_key, priv_key);
     return 1;
-}
 
+ err:
+    if (dsa_p)
+        BN_free(dsa_p);
+    if (dsa_q)
+        BN_free(dsa_q);
+    if (dsa_g)
+        BN_free(dsa_g);
+    if (priv_key)
+        BN_free(priv_key);
+    if (dummy_pub_key)
+        BN_free(dummy_pub_key);
+    return 0;
+}
 
 int get_dss_public_key(ErlNifEnv* env, ERL_NIF_TERM key, DSA *dsa)
 {