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
