[pulseaudio-discuss] [PATCH v4 1/2] raop: add error handling to rsa_encrypt()

[Tanu Kaskinen]

When reviewing another change in rsa_encrypt(), Felipe Sateler pointed
out some deficiencies in error handling. This patch adds error handling
for all openssl calls in rsa_encrypt().

This patch doesn't propagate the error all the way up to the
pa_rtsp_client owner, because there's no mechanism for doing that. I
could implement such mechanism myself, but I think it's better I don't
make such complex changes to the RAOP code, because I don't have any
RAOP hardware to test the changes. The result is that module-raop-sink
will just sit around without doing anything. I think this is still
better than having no error handling at all.
---
 src/modules/raop/raop_client.c | 60 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

Changes in v4:
 * Give credit to Felipe in the commit message.
 * Remove error message ambiguity in the two cases of BN_bin2bn()
   failures.

diff --git a/src/modules/raop/raop_client.c b/src/modules/raop/raop_client.c
index 31e0fdc..46147b6 100644
--- a/src/modules/raop/raop_client.c
+++ b/src/modules/raop/raop_client.c
@@ -176,19 +176,64 @@ static int rsa_encrypt(uint8_t *text, int len, uint8_t *res) {
     uint8_t exponent[8];
     int size;
     RSA *rsa;
-    BIGNUM *n_bn;
-    BIGNUM *e_bn;
+    BIGNUM *n_bn = NULL;
+    BIGNUM *e_bn = NULL;
+    int r;
 
     rsa = RSA_new();
+    if (!rsa) {
+        pa_log("RSA_new() failed.");
+        goto fail;
+    }
+
     size = pa_base64_decode(n, modules);
+
     n_bn = BN_bin2bn(modules, size, NULL);
+    if (!n_bn) {
+        pa_log("n_bn = BN_bin2bn() failed.");
+        goto fail;
+    }
+
     size = pa_base64_decode(e, exponent);
+
     e_bn = BN_bin2bn(exponent, size, NULL);
-    RSA_set0_key(rsa, n_bn, e_bn, NULL);
+    if (!e_bn) {
+        pa_log("e_bn = BN_bin2bn() failed.");
+        goto fail;
+    }
+
+    r = RSA_set0_key(rsa, n_bn, e_bn, NULL);
+    if (r == 0) {
+        pa_log("RSA_set0_key() failed.");
+        goto fail;
+    }
+
+    /* The memory allocated for n_bn and e_bn is now managed by the RSA object.
+     * Let's set n_bn and e_bn to NULL to avoid freeing the memory in the error
+     * handling code. */
+    n_bn = NULL;
+    e_bn = NULL;
 
     size = RSA_public_encrypt(len, text, res, rsa, RSA_PKCS1_OAEP_PADDING);
+    if (size == -1) {
+        pa_log("RSA_public_encrypt() failed.");
+        goto fail;
+    }
+
     RSA_free(rsa);
     return size;
+
+fail:
+    if (e_bn)
+        BN_free(e_bn);
+
+    if (n_bn)
+        BN_free(n_bn);
+
+    if (rsa)
+        RSA_free(rsa);
+
+    return -1;
 }
 
 static int aes_encrypt(pa_raop_client *c, uint8_t *data, int size) {
@@ -270,6 +315,15 @@ static void rtsp_cb(pa_rtsp_client *rtsp, pa_rtsp_state state, pa_headerlist *he
 
             /* Now encrypt our aes_public key to send to the device. */
             i = rsa_encrypt(c->aes_key, AES_CHUNKSIZE, rsakey);
+            if (i < 0) {
+                pa_log("rsa_encrypt() failed.");
+                pa_rtsp_disconnect(rtsp);
+                /* FIXME: This is an unrecoverable failure. We should notify
+                 * the pa_raop_client owner so that it could shut itself
+                 * down. */
+                return;
+            }
+
             pa_base64_encode(rsakey, i, &key);
             rtrimchar(key, '=');
             pa_base64_encode(c->aes_iv, AES_CHUNKSIZE, &iv);
-- 
2.10.1