On Thu, 2016-11-03 at 09:54 -0300, Felipe Sateler wrote: > On 3 November 2016 at 09:30, Tanu Kaskinen <tanuk at iki.fi> wrote: > > Openssl 1.1.0 made all structs opaque, which caused a build failure in > > raop_client.c. The struct member assignments are now replaced with a > > call to RSA_set0_key(). > > > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96726 > > --- > > > > Changes in v2: > > * More verbose comment. > > * Implement RSA_set0_key() as a function instead of a macro. > > * Use pa_assert_se() instead of pa_assert(). > > > > As before, I have tested that the code compiles against old and new openssl > > versions, but I have not tested that the code works. > > > > src/modules/raop/raop_client.c | 22 ++++++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/src/modules/raop/raop_client.c b/src/modules/raop/raop_client.c > > index 3b6c36e..7a7bb8a 100644 > > --- a/src/modules/raop/raop_client.c > > +++ b/src/modules/raop/raop_client.c > > @@ -68,6 +68,21 @@ > > > > #define RAOP_PORT 5000 > > > > +/* Openssl 1.1.0 broke compatibility. Before 1.1.0 we had to set RSA->n and > > + * RSA->e manually, but after 1.1.0 the RSA struct is opaque and we have to use > > + * RSA_set0_key(). RSA_set0_key() is a new function added in 1.1.0. We could > > + * depend on openssl 1.1.0, but it may take some time before distributions will > > + * be able to upgrade to the new openssl version. To insulate ourselves from > > + * such transition problems, let's implement RSA_set0_key() ourselves if it's > > + * not available. */ > > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > > +static int RSA_set0_key(RSA *r, BIGNUM *n, BIGNUM *e, BIGNUM *d) { > > + r->n = n; > > + r->e = e; > > + return 1; > > > I'm sorry I did not point this out before. I've looked at the > documentation of BN_bin2bn, and it can return NULL in case of failure. > The real version of RSA_set0_key will return 0 if passed NULL 'n' and > 'e', thus failing the assert. I'm not sure failing to 'BN_bin2bn' is a > recoverable error so I don't think that it is bad that the assertion > will fail. This new method does not fail the assert, even when it > couldn't correctly set the key. > > This code was buggy before this change, so maybe this code could go > as-is and fix this later (either by adding checks, or requiring a > new-enough openssl). It would probably be safe to wrap the BN_bin2bn() calls in assertions, because they should not need to talk to the kernel except when allocating memory, but since the error conditions aren't documented, I think it's more correct to not use assertions. This is true also for RSA_set0_key(). My preference would be to handle errors gracefully, but it looks like it's not trivial to implement, and I don't have motivation for spending time on that. I think having assertions is better than nothing, so I'll submit v3 with assertions for BN_bin2bn() and RSA_set0_key() plus a FIXME comment. -- Tanu https://www.patreon.com/tanuk
