Hello Selva,
On Tue, Jan 16, 2018 at 12:10 AM, Selva Nair <selva.n...@gmail.com> wrote:
>
>
> On Mon, Jan 15, 2018 at 5:33 PM, Emmanuel Deloget <log...@free.fr> wrote:
>
>> 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
>> :)
>>
>
> Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I
> was targeting only 1.1.0+ that's how most of those checks would have been
> done, isn't? Makes life easier, code simpler as I have said before..
>
> Just throwing in my 0.02c, feel free to ignore.
>
> Best,
>
> Selva
>
OpenSSL 1.1.0+ and previous versions allows you to create a typed EVP_PKEY
without any key inside. For example, you can do
EVP_PKEY *pkey = EVP_PKEY_new();
EVP_PKEY_set_type(pkey, EVP_PKEY_RSA);
This is plainly valid: the key is prepared for later use.
So testing for EVP_PKEY_id() is not sufficient to make sure that current
and future code won't mess with us. To be extra sure, one can do
if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey)) { ... }
Which is exactly what the current code does. Yet the idea (at least mine)
is to remove that to simplify code a bit.
Now, I agree, this is quite easy to read and to understand.
We have 3 solutions:
1) keep the code as is
if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL) { ...
}
2) remove EVP_PKEY_id() but keep a test on EVP_PKEY_getO_RSA()
if (EVP_PKEY_get0_RSA(pkey) != NULL) {
RSA *rsa = EVP_PKEY_get0_RSA(pkey);
...
}
3) rework it as proposed in the patch
RSA *rsa = NULL;
if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }
While I prefer the latest, I can understand that it might not be the best
solution. I don't have a strong opinion on the subject (that's why it's a
separate patch ;)).
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
