Hello Steffan,
On Sun, Jan 14, 2018 at 11:26 AM, Steffan Karger <stef...@karger.me> wrote:
> Hi,
>
> On 12-01-18 22:37, Emmanuel Deloget wrote:
> > Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as
> > the same check is also performed in the later.
> >
> > We also make the code a bit better by not calling the various
> > EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to
> > avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).
>
> Checking double is a bit silly, but we can fix that by simply removing
> the "EVP_PKEY_id() == ... && " occurrences. (That still allows us to
> remove it from the compat wrapper.)
>
> I'm not sure that moving the variables outside the if() scope actually
> improves the code. At least I think the original flow is easier to
> read. Mostly due to the #ifdef noise, but still. In a path that's not
> performance-critical, I prefer readability over performance.
>
Well, to be honest, I was definitely not trying to make any kind of
performance gain :)
The whole idea was to make code easier to read and to remove some now weird
duplication of functions (the kind of duplication that will come and bites
you later).
For the variables outside the ifs, the next C standard should allow us to
write something like:
if ((RSA *rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) {
...
That should be easier to read (but then, I'm not sure when this will be
considered as widespread ; I guess we're slated for 2025 or so :))
Anyway, moving the variables within the if means we're going to call
EVP_PKEY_get0_RSA() twice which, I feel, is not a good thing to do. But
even if I'm not very comfortable with this, I can still propose a patch
that des it (it's lighter than the currently proposed change). You tell me
:)
>
> > Signed-off-by: Emmanuel Deloget <log...@free.fr>
> >
> > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> > index 711bba11..7943fb2c 100644
> > --- a/src/openvpn/ssl_openssl.c
> > +++ b/src/openvpn/ssl_openssl.c
> > @@ -1699,22 +1699,13 @@ print_details(struct key_state_ssl *ks_ssl,
> const char *prefix)
> > EVP_PKEY *pkey = X509_get_pubkey(cert);
> > if (pkey != NULL)
> > {
> > - if ((EVP_PKEY_id(pkey) == EVP_PKEY_RSA) &&
> (EVP_PKEY_get0_RSA(pkey) != NULL))
> > - {
> > - RSA *rsa = EVP_PKEY_get0_RSA(pkey);
> > - openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
> > - RSA_bits(rsa));
> > - }
> > - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_DSA) &&
> (EVP_PKEY_get0_DSA(pkey) != NULL))
> > - {
> > - DSA *dsa = EVP_PKEY_get0_DSA(pkey);
> > - openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
> > - DSA_bits(dsa));
> > - }
> > + RSA *rsa = NULL;
> > + DSA *dsa = NULL;
> > #ifndef OPENSSL_NO_EC
> > - else if ((EVP_PKEY_id(pkey) == EVP_PKEY_EC) &&
> (EVP_PKEY_get0_EC_KEY(pkey) != NULL))
> > + EC_KEY *ec = NULL;
> > +
> > + if ((ec = EVP_PKEY_get0_EC_KEY(pkey)) != NULL)
> > {
> > - EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey);
> > const EC_GROUP *group = EC_KEY_get0_group(ec);
> > const char* curve;
> >
> > @@ -1726,9 +1717,19 @@ print_details(struct key_state_ssl *ks_ssl, const
> char *prefix)
> >
> > openvpn_snprintf(s2, sizeof(s2), ", %d bit EC, curve:
> %s",
> > EC_GROUP_order_bits(group), curve);
> > -
> > - }
> > + } else
> > #endif
> > + if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL)
> > + {
> > + openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
> > + RSA_bits(rsa));
> > + }
> > + else if ((dsa = EVP_PKEY_get0_DSA(pkey)) != NULL)
> > + {
> > + openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
> > + DSA_bits(dsa));
> > + }
> > +
> > EVP_PKEY_free(pkey);
> > }
> > X509_free(cert);
> >
>
> -Steffan
>
Best regards,
-- Emmanuel Deloget
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
