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

[Felipe Sateler]

On 4 November 2016 at 09:43, Tanu Kaskinen <tanuk at iki.fi> wrote:
> While making another change in rsa_encrypt(), I noticed that it had no
> error handling. I decided to add some.
>
> 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(-)
>
> diff --git a/src/modules/raop/raop_client.c b/src/modules/raop/raop_client.c
> index 864c558..32cd316 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("BN_bin2bn() failed.");

I think this should note which instance of BN_bin2n failed? The
documentation does not mention why can bin2bn fail, other than memory
allocations. If there are other reasons, it probably will be useful to
know which call was that 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("BN_bin2bn() failed.");
> +        goto fail;
> +    }
> +
> +    r = RSA_set0_key(rsa, n_bn, e_bn, NULL);
> +    if (r == 0) {

This seems to be the only use of r. I see elsewhere that usages of the form:

if (!some_func()) {
    // log or do something
    goto fail;
}

Is there a convention about this?

> +        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;

Other than the log, this seems redundant. goto fail will do the same
as not doing 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
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss



-- 

Saludos,
Felipe Sateler