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

[Tanu Kaskinen]

On Fri, 2016-11-04 at 11:18 -0300, Felipe Sateler wrote:
> 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.

True. I'll fix this.

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

The coding style document doesn't say anything about this. There are
probably more instances of "if (foo() < 0)" than "if (r < 0)", so we
could add this to the style document, if people feel that enforcing the
current dominant approach would be beneficial. Personally I find the
"if (r < 0)" approach easier to read.

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

True, but I feel it's nicer to handle all errors in the fail section.
That way the reader doesn't have to wonder whether there's an error
check missing.

-- 
Tanu

https://www.patreon.com/tanuk