Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Check for the specific in-use version of OpenSSL when working with libcurl #32483

Merged
merged 1 commit into from
Sep 26, 2018

Conversation

bartonjs
Copy link
Member

Rather than check a generic 1.0/1.1, test for the specific library version that
the crypto shim has loaded. This makes things work when both libcurl
and the crypto shim are using OpenSSL 1.1 and also prevents a state where two
different copies of the library (at different patch versions) are utilized.

Fixes #32343.
Related to #32006 / #9855.

…bcurl

Rather than check a generic 1.0/1.1, test for the specific library version that
the crypto shim has loaded.  This makes things work when both libcurl
and the crypto shim are using OpenSSL 1.1 and also prevents a state where two
different copies of the library (at different patch versions) are utilized.
@bartonjs bartonjs added this to the 3.0 milestone Sep 26, 2018
@bartonjs bartonjs self-assigned this Sep 26, 2018
@bartonjs
Copy link
Member Author

@dotnet-bot Test Outerloop Linux x64 Debug Build please

internal const string SecureTransportDescription = "SecureTransport";
internal const string LibreSslDescription = "LibreSSL";

#if !SYSNETHTTP_NO_OPENSSL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is defined in the tests project? The tests pull in this file? Ugh, but ok.

// Ask the product how it feels about this.
Type interopHttp = typeof(HttpClient).Assembly.GetType("Interop+Http");
PropertyInfo hasMatchingOpenSslVersion = interopHttp.GetProperty("HasMatchingOpenSslVersion", BindingFlags.Static | BindingFlags.NonPublic);
return (bool)hasMatchingOpenSslVersion.GetValue(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the tests are pulling in the file anyway, do we need to exclude that code when built in the tests, or could we just use that rather than using reflection?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests pull in that one file, but not the rest of the work for the HttpNative/CryptoNative initializer sequencing, and leaving it that way helps to avoid a Heisenbug where the tests end up doing the initialization on behalf of the product and the product eventually neglects to do so.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We will still prefer 1.1 for crypto & SskStream, right?

@bartonjs
Copy link
Member Author

We will still prefer 1.1 for crypto & Ss[l]tream, right?

For 3.0, yep. This just makes it so that our "can we use OpenSSL via libcurl" matches what we picked, curl's bound library won't influence our loader preference.

@bartonjs
Copy link
Member Author

@wfurt and I talked about the tests that are failing, and it seems like we're in a better state with this checked in while the remaining OuterLoop failures get investigated, so merging it as-is (InnerLoop is fine)

@bartonjs bartonjs merged commit d33f843 into dotnet:master Sep 26, 2018
@rmkerr
Copy link
Contributor

rmkerr commented Sep 27, 2018

I'm seeing a bunch of new failures in the System.Net tests today that I think might be related to this change:
image
Both tests are failing because no exceptions are thrown when a PlatformNotSupportedException is expected.

It would be nice to figure this out, since these two tests alone caused more failures today than we've seen any other day in the last two weeks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants