Consistently load OpenSSL libraries among different platforms

[KostasTsiounis]

As part of this pull request, these changes are performed:

• Allow loading specific user-defined OpenSSL library using the -Djdk.native.openssl.lib option.
• OpenSSL library loading is consolidated into a single file using ifdefs.
• The platform-specific MD files are no longer needed and are thus deleted.
• Additional traces are added to indicate the attempts and actual OpenSSL library loaded.
• The location of the library loaded is printed.

Moreover, the order of preference for loading a library is updated to follow this order:

1. Explicitly load what was specified via JVM property. Fail if loading fails.
2. Search within the Semeru directories for a bundled version.
3. Search the system for existing libraries and attempt to find the higher version.
4. If all of the previous steps fail, revert to original Java implementation for crypto.

Co-authored by: Paritosh Kumar [email protected]
Co-authored by: Kostas Tsiounis [email protected]

Signed-off-by: Kostas Tsiounis [email protected]

[pshipton]

The commit comment mentions jdk.openssl.libName

[pshipton]

There is a typo as well - porperty

[KostasTsiounis]

The commit comment mentions jdk.openssl.libName

Updated to what we discussed.

There is a typo as well - porperty

Fixed that.

[keithc-ca]

Please apply formatting and style feedback throughout.

[keithc-ca]

Using 0 may be slightly cheaper (the value isn't used).

[keithc-ca]

I think it better to add it later if we find it's needed.

[keithc-ca]

Some C++-style comments remain; see lines 3622, 3628, 3664, etc.

[KostasTsiounis]

Searched and replaced all C++-style comments.

[keithc-ca]

At some point, this should be squashed and the commit message updated: some lines are too long. Bullet points should be continued with clarifying indentation, for example:

- Additional traces are added to indicate the attempts and actual
  OpenSSL library laoded.

The summary line should not end with a period.

[KostasTsiounis]

At some point, this should be squashed and the commit message updated: some lines are too long. Bullet points should be continued with clarifying indentation, for example:

- Additional traces are added to indicate the attempts and actual
  OpenSSL library laoded.

The summary line should not end with a period.

I have squashed and updated the commit message according to suggestions.

[keithc-ca]

Please rebase and resolve the merge conflict.

[KostasTsiounis]

Please rebase and resolve the merge conflict.

Rebased to head.

[keithc-ca]

I think the entry for the symlink should be included as it was before this change.

[KostasTsiounis]

Moved to the end of the list.

[keithc-ca]

That's consistent with the old behavior, nor with the comment above (even if it were more than just a comment here).

[keithc-ca]

Do you have a reference to support the claim that symbolic links are not supported on macOS?

[KostasTsiounis]

We found in discussion like this, that Apple has disabled generic loads after MacOS 11. We tried ourselves too and were able to reproduce it.

Also, I'm confused. Do we want this in the beginning of the list, as it would be if we actually used it, or the end, like it was previously?

[keithc-ca]

Apple's release notice at https://developer.apple.com/documentation/macos-release-notes/macos-big-sur-11_0_1-release-notes, says

check for library presence by attempting to dlopen() the path, which will correctly check for the library in the cache

Since we're using dlopen(), I don't see a problem. Have you observed otherwise? If listing the name of what is expected to by a symlink doesn't cause a problem, then it should be first to be consistent with other platforms.

[KostasTsiounis]

If you see this and this comments, you'll see what we did to test and the failure we were getting.

It was producing a warning that was turned to be fatal after MacOS 11. I think it's because the libcrypto.dylib is not a soft link by default (I've seen hacks where one would delete the existing one and create a soft link manually and it would work), but rather it's expected to produce a failure. It could work if someone has performed those extra steps, but a failure is produced if the default files are in place.

[keithc-ca]

That's better, but the */ of a multi-line comment should be on a line by itself.
Please correct the spelling of "macOS", and I think It should say "on" (a platform) rather than "in".
Another approach would be to mention the exception for macOS when explaining why symlinks are listed first.

[KostasTsiounis]

You mean in the overall comment?

[KostasTsiounis]

Added wording as a note in the comment above the libraries.

[keithc-ca]

This is missing error handling: GetStringUTFChars() may return NULL. Suggest:

    if (NULL != jlibname) {
        const char *clibname = (*env)->GetStringUTFChars(env, jlibname, NULL);
        if (NULL == clibname) {
            if (traceEnabled) {
                fprintf(stderr, "Failed to get jdk.native.openssl.lib value.\n");
                fflush(stderr);
            }
            return -1;
        }
        if ('\0' == clibname[0]) {
            if (traceEnabled) {
                fprintf(stderr, "The jdk.native.openssl.lib property is not set.\n");
                fflush(stderr);
            }
        } else {
            crypto_library = load_crypto_library(traceEnabled, clibname);
            if (NULL == crypto_library) {
                if (traceEnabled) {
                    fprintf(stderr, "OpenSSL library specified in jdk.openssl.lib couldn't be loaded.\n");
                    fflush(stderr);
                }
                (*env)->ReleaseStringUTFChars(env, jlibname, clibname);
                return -1;
            }
        }
        (*env)->ReleaseStringUTFChars(env, jlibname, clibname);
    }

Notice the previously missing calls to ReleaseStringUTFChars() on failure paths.

[KostasTsiounis]

Changed to suggestion.

[keithc-ca]

More missing error handling:

    if (NULL != jhomepath) {
        chomepath = (*env)->GetStringUTFChars(env, jhomepath, NULL);
        if (NULL == chomepath) {
            if (traceEnabled) {
                fprintf(stderr, "Failed to get java.home value.\n");
                fflush(stderr);
            }
            return -1;
        }
    }

[KostasTsiounis]

Changed to suggestion.

[keithc-ca]

clibname is released above (in my suggestions), chomepath cannot be released if jhomepath is NULL.

    if (NULL != jhomepath) {
        (*env)->ReleaseStringUTFChars(env, jhomepath, chomepath);
    }

[KostasTsiounis]

Changed to suggestion.

[keithc-ca]

Some problems remain:

• I don't see a definition of size which means this won't compile
• only one entry in libNamesWithPath is used at a time, so there's no need for an array
• libNamesWithPath and it's entries are not freed, nor is libPath
• the hard-coded value 16 should be replaced by sizeof(suffix) where suffix is suitably defined:

#if defined(_WIN32)
    static const char pathSuffix[] = "\\bin\\";
#else /* defined(_WIN32) */
    static const char pathSuffix[] = "/lib/";
#endif /* defined(_WIN32) */

• the static locals can be declared first, allowing non-static locals to be initialized directly

    const size_t numOfLibs = sizeof(libNames) / sizeof(libNames[0]);
#if defined(_AIX)
    const size_t num_of_generic = 4;
#elif defined(__linux__) /* defined(_AIX) */
    const size_t num_of_generic = 1;
#else /* defined(__linux__) */
    const size_t num_of_generic = 0;
#endif /* defined(_AIX) */

    void *result = NULL;
    void *prevResult = NULL;
    size_t i = 0;
    long tempVersion = 0;
    long previousVersion = 0;

• this could do a single allocation for libPath that has sufficient room for chomepath, the subdirectory, and the longest library name, replacing the library name for each iteration (but that might be harder to understand)

[KostasTsiounis]

Regarding the problems, here is what I did:

• I renamed size to numOfLibs, but I missed that spot.
• I avoid creating an array. I just use a single libNameWithPath variable in the loop.
• I added free commands for both where appropriate.
• I substituted the hardcoded value with appropriately calculated sizes.
• I moved declarations around as suggested to have the non-static ones be initialized directly.
• I think that would add complexity that wouldn't really get us much since this is only run in the beginning and the loop stops as soon as a library is found.

[keithc-ca]

Please remove the redundant parentheses and add 1 for the NUL terminator.

            char *libNameWithPath = (char *)malloc(path_len + file_len + 1);

[KostasTsiounis]

Removed parentheses and added 1.

[keithc-ca]

libNameWithPath cannot be NULL here (see lines 700-705).

[KostasTsiounis]

Yes, makes sense. Removed check and free.

[keithc-ca]

Line 717 must stay (it was just checking for NULL that was unnecessary).

[KostasTsiounis]

Sorry, my bad. Re-added free.

[keithc-ca]

A blank line before lines 683 and 685 would be welcome (and consistent with lines 660 and 669).

[KostasTsiounis]

Blank lines added.