-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Find the correct OpenSSL library for FreeBSD #34898
Conversation
Tagging @bartonjs as an area owner. If you would like to be tagged for a label, please notify danmosemsft. |
#ifdef __FreeBSD__ | ||
DlOpen(MAKELIB("11")); | ||
#else | ||
DlOpen(MAKELIB("1.1")); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little weird that you've replaced the primary recommended name, left fallback names, and then added one more fallback.
I'd sort of expect either the whole list to be replaced, or both library probes to be additive... but one replace, one add is weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As FreeBSD support is WIP at the moment I guess I was trying to make a targeted change to the bits that didn't work on my set-up.
Perhaps this function should just work through a list of versions in order of preference to match on the best library. It would be good if the list wasn't in compiled code but I really have no idea how many possible library versions are needed nor how often they change.
I'm happy to drop this for a better/more standard solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The escape valve is the CLR_OPENSSL_VERSION_OVERRIDE environment variable. There's not really any other config model that we can really have at this point.
I'd just expect this to be either
- All of the current things, then the FreeBSD things, unconditional (or an argument for why the FreeBSD ones come first)
- All of the current things, then the FreeBSD things, conditionally compiled for FreeBSD (possibly with the FreeBSD ones first)
- The entirety of this list replaced for FreeBSD.
so the versionOverride stuff, then
#if __FreeBSD__
if (libssl == NULL)
{
// OpenSSL 1.1.1 on FreeBSD x.y
DlOpen(MAKELIB("11"));
}
if (libssl == NULL)
{
...
}
#else
// all of the current guarded DlOpen(MAKELIB()) calls
#endif
return libssl != NULL;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the #ifdef entirely. I hope this makes sense. I am making the assumption that any non-FreeBSD system that fell through without a match previously will continue to do so. If you disagree, let me know and I'll fence the new code.
@danmosemsft is that offer open to contributors? |
I don't see why not - but I have to do this manually, so perhaps only for the most regular contributors (surely there is some external solution for hooking label changes?). Which labels would you like to be notified for @vcsjones ? |
@vcsjones actually - I take that back. It would show up that you're the "area owner" and I think that might cause confusion. Let me think about this. Let me know if you find another mechanism, too. |
Even though you are helping be an area owner, in a sense ! Which we appreciate. Hmm |
I get this on 11.3: So for legacy 1.0, the link is '.8'
I'm wondering if we could/should just fallback to `libssl.so' without version?
on 12.1 I get
There seems to be difference between 12.1 and 11.3 - both can use OpenSSL 1.1.1 but the linkage is different. Maybe the sequence should be |
Thanks @wfurt I had not looked at other versions of FreeBSD yet. |
Just for the record, I have spun up VMs for 12.1 and a current development snapshot of 13.0. I don't know the background on how these library numbers are assigned but unfortunately it doesn't look like we can say Of course |
I would be fine with that order. That allows OpensSSL From ports be first. Since none of the existing code uses |
The version of OpenSSL installed will depend on the version of FreeBSD and whether OpenSSL has been updated from the FreeBSD Ports collection. The order of attempted loading is: libssl.so.11 libssl.so.111 libssl.so.8 Please see the issue for further detail. dotnet#34897
0e48f6c
to
9662d17
Compare
I have updated the code and removed the #ifdef so the FreeBSD specific code will only run if a library hasn't already been found. |
The failure looks like #2271; and this shouldn't have any real way of exacerbating that problem. |
This is a minor change, it builds and works on my setup - previously dotnet crashed on first-run when trying to send telemetry.
OpenSSL 1.1.1 -> libssl.so.11
OpenSSL 1.0.2 -> libssl.so.8
#34897
@wfurt