Repush of pull request #98 due to strawberry hickup.

[chincheta0815]

No description provided.

[h-vn]

Thanks for looking at this. I'd say we could even drop the #else part that uses RSA_generate_key(). This would mean 0.9.8 is the oldest supported version. Also, I gave this a quick spin on different LibreSSL versions and with the following modification the patches version compiles with 2.2.1 and later:

#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER)) || (LIBRESSL_VERSION_NUMBER >= 0x2070000fL)

However, lets see pull request #95 about what to do with LibreSSL version checks.

[h-vn]

@chincheta0815 in your Perl RT ticket at https://rt.cpan.org/Public/Bug/Display.html?id=127593 you mention that compilation failed because of missing symbol. What system did you try compiling it?

Since it works with latest OpenSSL master, it was probably something related to the system you were using. For example, OpenSSL was compiled to exclude deprecated APIs. Your patch is fine, but I thought it would be good to know how different systems are configured.

[chincheta0815]

The error was a "relocation error" saying that "symbol" for "RSA_generate_key_ex" was not found. The system was the latest stable OmniOSCE (Illumos). This system has transitioned to OpenSSL 1.1 by default. You can set it back to OpenSSL 1.0, but that is not the best choice. I also think that it was compiled without using deprecated API. Nevertheless, the default system packages of OmniOSCE are involved, so the patch seems reasonable (also for Android).

It tried to cover everything in a general manner, using the info in the OpenSSL docs:

• https://www.openssl.org/docs/man1.1.1/man3/RSA_generate_key.html
• https://www.openssl.org/docs/man1.1.1/man3/BN_GENCB_new.html

Also, https://www.openssl.org/docs/man1.1.1/man3/BN_GENCB_new.html states that "BN_GENCB callback;" is remove not deprecated. Hope this helps.

[chincheta0815]

Sorry for closing.

[chincheta0815]

Thanks for looking at this. I'd say we could even drop the #else part that uses RSA_generate_key(). This would mean 0.9.8 is the oldest supported version. Also, I gave this a quick spin on different LibreSSL versions and with the following modification the patches version compiles with 2.2.1 and later:

#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER)) || (LIBRESSL_VERSION_NUMBER >= 0x2070000fL)

However, lets see pull request #95 about what to do with LibreSSL version checks.

Just a comment: Well, skipping the #else part might be an idea, but is this really necessary? Since it won't change a lot support old version it might be an idea. Anyways the code would be more complex.

Do you wan me to change anything in my codebase and include your path or will/did you already do something?

[h-vn]

You could add the LibreSSL version checks to the line that now checks for OpenSSL 1.1.0. Then it would work with a number of LibreSSL versions too.

What comes to versions older than 0.9.8, all of them are now more than 12 years old. At some point support for those versions can go. However, we can keep this patch shorter by not changing multiple things at the same time.

One more thing: You mentioned that RSA_generate_key_ex was not found. I thought this is the current function and RSA_generate_key is the one that's deprecated.

[chincheta0815]

For the first two paragraph:
I will provide a patch in my code. Leaving the #else part in too make it not too complicated for now.

For the last paragraph:
I will check. All I remember is that it now works with the "_ex" version, and that the "without _ex" was not there anymore.

[h-vn]

The ifdefs look good now. I tested it with LibreSSL 2.2.1 up to 2.8.2 and OpenSSL master.

One thing I noticed is that return value checks were not done in the old code as they should be, for example checks for BN_new() and RSA_new() are missing and late. Your code adds BN_GENCB_new() which should be checked for failure too. Could you fix this at least?

[chincheta0815]

Sure I can.
I just need a little help by you.
The code would be

new_cb = BN_GENCB_new();
if(!new_cb) {
WHAT would be GOOD here?
}

I am a little confused by all the perl stuff. In C I would simply print "ERROR: could not allocate new_cb."
How would a correct statement look like?

If I got that I think I can easily add that for BN_new() and RSA_new()

Would it be as: https://github.com/chincheta0815/p5-net-ssleay/blob/46cc1aea99d1d17920cb427b880645e5706c70b5/SSLeay.xs#L1735
or
as: https://github.com/chincheta0815/p5-net-ssleay/blob/46cc1aea99d1d17920cb427b880645e5706c70b5/SSLeay.xs#L1604

[h-vn]

Thanks for adding the checks and croaks. I added a couple of more free calls before the croaks to free what was allocated before croak is needed.

[chincheta0815]

Thanks.
I always forget to free things. ;o)