HomeWildfire Games

Update libcurl to 7.59.0 on Windows and enable SSL support on Windows and macOS.

Description

Update libcurl to 7.59.0 on Windows and enable SSL support on Windows and macOS.
Refs #3004, #4362.

Event Timeline

elexis raised a concern with this commit.EditedOct 18 2018, 11:22 AM
elexis added a subscriber: elexis.

http://irclogs.wildfiregames.com/2018-05/2018-05-11-QuakeNet-%230ad.log
http://irclogs.wildfiregames.com/2018-05/2018-05-12-QuakeNet-%230ad.log

14:03 <@Itms> I didn't know that 1) OSX used static linking, so I didn't look into the lib build flags when I added libsodium and 2) that premake would assume dynamic linking when the dylibs are present.
14:03 <@Itms> Tobbi: The fix is here: https://github.com/na-Itms/0ad/tree/osx-fix

D1610

This commit now has outstanding concerns.Oct 18 2018, 11:22 AM

libidn2 issue reported at #5231.

2018-05/2018-05-12-QuakeNet-#0ad.log:14:03 <@Itms> Tobbi: The fix is here: https://github.com/na-Itms/0ad/tree/osx-fix

The --without-libidn2 flag seems unrelated to rP21683, so should warrant a separate commit or at least description.

Why didn't the libidn2 issue occur in the alpha 22 release?
The curl commit using libidn2 was already done at this point ( 2016-10):

https://github.com/curl/curl/commit/9c91ec778104ae3b744b39444d544e82d5ee9ece
9c91ec77 configure.ac (Daniel Stenberg 2016-10-12 09:01:06 +0200 3237) AC_HELP_STRING([--without-libidn2],[Disable libidn2 usage]),

Tobbi had used macOS 10.12 to build alpha 22:

2017-07/2017-07-22-QuakeNet-#0ad-dev.log:22:46 < Tobbi> elexis: 10.12

Wasn't it rather rP19848 in Jun 30 2017 changing from curl-7.46.0 to curl-7.54.0?
https://curl.haxx.se/download/

curl-7.46.0.tar.bz2 2015-12-02 08:04 3.3M
curl-7.54.0.tar.bz2 2017-04-19 07:45 2.5M

But then why did it not break when the diff was tested by Itms and Tobbi and after the commit by Tobbi when packaging the release?

2017-07/2017-07-21-QuakeNet-#0ad-dev.log:22:37 < Tobbi> https://mega.nz/#!JQkSQZiD!K3716LvoFb03vYe8H81YM_7tzp0Z0bjdj56HVJeCBfo

About dropping 10.7, I'm not sure if this is actually necessary or correct.

In curl_darwinssl.c of curl we see

/* macOS 10.5-10.7 supported TLS 1.0 only.

macOS 10.8 and later, and iOS 5 and later, added TLS 1.1 and 1.2.
macOS 10.13 and later, and iOS 11 and later, added TLS 1.3. */

So it would only be necessary if mod.io and UserReporter don't work with TLS 1.0.
But they do, as we can see with nmap --script ssl-enum-ciphers -p 443 mod.io:

TLSv1.0:
...

The drop of 10.7 was added to fix the reported build error:

2018-05/2018-05-11-QuakeNet-#0ad.log:21:01 < Tobbi> Original libcurl build error was https://pastebin.com/5AQkSzAT

vtls/darwinssl.c:1308:6: error: 'SSLSetProtocolVersionMax' is only available on macOS 10.8 or newer [-Werror,-Wunguarded-availability]

But if we look into lib/vtls/darwinssl.c there already are version guards:

#if CURL_BUILD_MAC_10_8 || CURL_BUILD_IOS
if(SSLSetProtocolVersionMax != NULL) {

build-osx-libs.sh compiles curl to use 10.9, which explains why the safeguard is true.

export MIN_OSX_VERSION=${MIN_OSX_VERSION:="10.9"}

But build-osx-libs.sh passes 10.7 to premake which generates the makefiles for curl:

export MIN_OSX_VERSION=${MIN_OSX_VERSION:="10.7"}
...
# Pass OS X options through to Premake
(./clean-workspaces.sh && SYSROOT="$SYSROOT" MIN_OSX_VERSION="$MIN_OSX_VERSION" ./update-workspaces.sh --macosx-bundle="$BUNDLE_IDENTIFIER" --sysroot="$SYSROOT" --macosx-version-min="$MIN_OSX_VERSION") >> $build_log 2>&1 || die "update-workspaces.sh failed!"

So it seems the logic is flawed and it's just a coincidence that this breaks with this library and this version while the problem could reoccur with any library and any version, nay?
If so, there should be at least a comment to keep both versions in sync if the code logic isn't fixed.
If so, it should be 10.8 in both files and rP15410, rP15447, rP15461 that changed it to 10.8 and 10.9 in one file while keeping 10.7 in the other were broken already?

Itms requested verification of this commit.Dec 26 2018, 10:21 AM

@elexis Are we good with this? The remaining issues should be fixed by upgrading premake and committing a few patches. If I'm wrong we could really use a ticket to document our findings.

This commit now requires verification by auditors.Dec 26 2018, 10:21 AM
elexis removed an auditor: elexis.Dec 26 2018, 12:10 PM

I don't recall what the issue was other than different places using different, possibly conflicting version numbers. Can't verify that it works as intended, but I recon that you reworked that part, so I hope it should be good now. Thanks again for grappling with that!

This commit no longer requires audit.Dec 26 2018, 12:10 PM