build: remove configure checks for win libraries we don't link against

fanquake commented at 2:46 am on December 13, 2019: member

While cross compiling, HOST=x86_64-w64-mingw32, none of these libs actually seem to be passed to the linker. i.e tailing a build with make -j5 V=1 | rg -i 'mingwthrd|winspool|rpcrt4|crypt32'.

I’m not 100% sure about crypt32, even though the majority of our Windows cryptography usage, i.e CryptAcquireContextW or CryptGenRandom is provided by advapi32.

Note that rpcrt4 and mingwthrd are already missing from the MSVC build, so we can sync the remainder once it’s clear what’s actually needed. Hopefully sipsorcery can add some MSVC insight.

fanquake added the label Windows on Dec 13, 2019

fanquake added the label Build system on Dec 13, 2019

fanquake added the label Needs gitian build on Dec 13, 2019

fanquake requested review from sipsorcery on Dec 13, 2019

DrahtBot commented at 3:20 am on December 13, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17213 (gui: Add Windows taskbar progress by luke-jr)
#15382 (util: add runCommandParseJSON by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipsorcery approved

sipsorcery commented at 9:44 am on December 13, 2019: member

Looks fine to me. My understanding is most of the mingw libraries are simple wrappers that load the matching Windows dll at runtime. If the build works without referencing a library then it seems pretty safe to remove it.

sipsorcery commented at 9:45 am on December 13, 2019: member

ACK 1778e206c33a814c8c07d998196159b362263f16.

Note the appveyor build failed on the python functional tests which is nothing to do with this PR.

laanwj commented at 10:04 am on December 13, 2019: member

I’m not 100% sure about crypt32, even though the majority of our Windows cryptography usage, i.e CryptAcquireContextW or CryptGenRandom is provided by advapi32.

As for crypt32: we don’t use any kind of weak linking of symbols, I’d say if it links, it’s safe.

The only one that slightly scares me (that it can be dropped, not the change itself) is mingwthrd because in the deep past there have been issues with mingw not using the right threading runtime. ~~But I suspect this is simply because we require using the -posix variant of the tool chain (because of using C++11 threads, instead of using win32 threads directly), so the library is bypassed.~~

laanwj commented at 10:08 am on December 13, 2019: member

~~Hmm this has some information about the hack that mingwthrd does: https://gcc.gnu.org/ml/java/2006-07/msg00003.html~~ ~~it contains two functions which perform setup and cleanup based on loading/unloading notifications, so the symbols are not used directly.~~ ~~I’m not sure about this one! It might not be something that’s ever required to explicitly link against, but it feels risky somehow.~~

Oh, I missed the “none of these libs actually seem to be passed to the linker”—ok in that case, I don’t see why it’s checking for the library

So this is safe. These checks don’t cause linkage. They just check whether the library exists. I suppose the explicit check for mingwthrd was added at some point to be sure a thread-supporting mingw is used. Maybe @theuni still knows.

fanquake force-pushed on Dec 13, 2019

fanquake commented at 1:38 pm on December 13, 2019: member

Added some additional changes for the MSVC build system.

fanquake marked this as ready for review on Dec 13, 2019

sipsorcery commented at 1:54 pm on December 13, 2019: member

fgep’ing for the defines below does not get any results for the src directory. As far as the msvc build goes if the define is not used in the code it’s not used at all. They can be removed along with the ones already taken out.

It does actually seem a bit strange for any defines not to be used in the code no matter what the build system. I’d be surprised if the mingw makefile parses bitcoin_config.h to set it’s linker command.

 0/* Define to 1 if you have the `advapi32' library (-ladvapi32). */
 1#define HAVE_LIBADVAPI32 1
 2
 3/* Define to 1 if you have the `comctl32' library (-lcomctl32). */
 4#define HAVE_LIBCOMCTL32 1
 5
 6/* Define to 1 if you have the `comdlg32' library (-lcomdlg32). */
 7#define HAVE_LIBCOMDLG32 1
 8
 9/* Define to 1 if you have the `crypt32' library (-lcrypt32). */
10#define HAVE_LIBCRYPT32 1
11
12/* Define to 1 if you have the `gdi32' library (-lgdi32). */
13#define HAVE_LIBGDI32 1
14
15/* Define to 1 if you have the `imm32' library (-limm32). */
16#define HAVE_LIBIMM32 1
17
18/* Define to 1 if you have the `iphlpapi' library (-liphlpapi). */
19#define HAVE_LIBIPHLPAPI 1
20
21/* Define to 1 if you have the `kernel32' library (-lkernel32). */
22#define HAVE_LIBKERNEL32 1
23
24/* Define to 1 if you have the `mingwthrd' library (-lmingwthrd). */
25#define HAVE_LIBMINGWTHRD 1
26
27/* Define to 1 if you have the `mswsock' library (-lmswsock). */
28#define HAVE_LIBMSWSOCK 1
29
30/* Define to 1 if you have the `ole32' library (-lole32). */
31#define HAVE_LIBOLE32 1
32
33/* Define to 1 if you have the `oleaut32' library (-loleaut32). */
34#define HAVE_LIBOLEAUT32 1
35
36/* Define to 1 if you have the `rpcrt4' library (-lrpcrt4). */
37#define HAVE_LIBRPCRT4 1
38
39/* Define to 1 if you have the `rt' library (-lrt). */
40/* #undef HAVE_LIBRT */
41
42/* Define to 1 if you have the `shell32' library (-lshell32). */
43#define HAVE_LIBSHELL32 1
44
45/* Define to 1 if you have the `shlwapi' library (-lshlwapi). */
46#define HAVE_LIBSHLWAPI 1
47
48/* Define to 1 if you have the `ssp' library (-lssp). */
49#define HAVE_LIBSSP 1
50
51/* Define to 1 if you have the `user32' library (-luser32). */
52#define HAVE_LIBUSER32 1
53
54/* Define to 1 if you have the `uuid' library (-luuid). */
55#define HAVE_LIBUUID 1
56
57/* Define to 1 if you have the `winmm' library (-lwinmm). */
58#define HAVE_LIBWINMM 1
59
60/* Define to 1 if you have the `winspool' library (-lwinspool). */
61#define HAVE_LIBWINSPOOL 1
62
63/* Define to 1 if you have the `ws2_32' library (-lws2_32). */
64#define HAVE_LIBWS2_32 1
65
66/* Define to 1 if you have the `z ' library (-lz ). */
67#define HAVE_LIBZ_ 1

in build_msvc/common.init.vcxproj:119 in fd33d90b3f outdated

114@@ -115,7 +115,7 @@
115     <Link>
116       <SubSystem>Console</SubSystem>
117       <GenerateDebugInformation>true</GenerateDebugInformation>
118-      <AdditionalDependencies>crypt32.lib;Iphlpapi.lib;ws2_32.lib;Shlwapi.lib;kernel32.lib;user32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies)</AdditionalDependencies>
119+      <AdditionalDependencies>Iphlpapi.lib;ws2_32.lib;Shlwapi.lib;kernel32.lib;user32.lib;gdi32.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies)</AdditionalDependencies>

sipsorcery commented at 1:56 pm on December 13, 2019:

Can confirm removing those two libraries doesn’t break the build.

fanquake added the label Waiting for author on Dec 13, 2019

fanquake commented at 4:55 pm on December 13, 2019: member

Discussed with @theuni. Going to add specific symbols to each of the AC_CHECK_LIB calls in place of main.

DrahtBot removed the label Needs gitian build on Dec 14, 2019

laanwj commented at 1:54 pm on December 15, 2019: member

Discussed with @theuni. Going to add specific symbols to each of the AC_CHECK_LIB calls in place of main.

Right. I’ve started to wonder: why are these checks there in the first place? Why does it check for libraries it doesn’t directly link against? (and if it’s libraries it does link against, would it be an idea to combine the check and adding the library in the same place, so it’s easier to see why a certain library is there?)

fanquake force-pushed on Dec 31, 2019

fanquake removed the label Waiting for author on Dec 31, 2019

fanquake commented at 6:55 pm on December 31, 2019: member

Incorporated suggestions from @sipsorcery and added specific symbols to some of the AC_CHECK_LIB calls.

sipsorcery commented at 10:37 pm on December 31, 2019: member

tACK (msvc and mingw on Win10) 890baac6cd6663974344553113de5d99de0e05ea.

MarcoFalke deleted a comment on Jan 2, 2020

MarcoFalke added the label Needs gitian build on Jan 2, 2020

DrahtBot commented at 1:28 am on January 3, 2020: member

Gitian builds

File	commit 35fff5be60e853455abc24713481544e91adfedb(master)	commit 65c7a3f6c1afbd33623fc7d8c962cdfda32b174e(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz	`1a1b79c30ee23b1e...`	`6e3d069e49d0568c...`
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz	`4f5477e988e15339...`	`7f1e8fe70b2b9b5c...`
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz	`907d28fc95980468...`	`61f66840cbfeb2bb...`
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz	`e55899ead1a7ff56...`	`3820fc0cd9640855...`
bitcoin-0.19.99-i686-pc-linux-gnu-debug.tar.gz	`95fa4bccc8b22ac0...`	`ca541891a3c32a98...`
bitcoin-0.19.99-i686-pc-linux-gnu.tar.gz	`340c796226da90c7...`	`d37ae258b32889d5...`
bitcoin-0.19.99-osx-unsigned.dmg	`1c5ab941efd90df4...`	`d343969d150f639c...`
bitcoin-0.19.99-osx64.tar.gz	`e0d2d4ef7befbf1f...`	`ad92ec957ef6a02f...`
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz	`7fb3888850d15c57...`	`91c0c886a5e0b811...`
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz	`c6bd3037c22bbde5...`	`dca861193424cdd0...`
bitcoin-0.19.99-win64-debug.zip	`a2369f394ffcb551...`	`9b23196ccac0dc5f...`
bitcoin-0.19.99-win64-setup-unsigned.exe	`ffc23fa5abc3ca5f...`	`125ddc9039515ac1...`
bitcoin-0.19.99-win64.zip	`2d13e80f0011ce7d...`	`8ef5b7ae1a8aa1ce...`
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz	`2a4aa0a81c4d2591...`	`780d73b51acc3d67...`
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz	`8940a74e71b82107...`	`0170a14cbf1b191e...`
bitcoin-0.19.99.tar.gz	`7703c9c861b5b755...`	`ff916a409a89a239...`
bitcoin-core-linux-0.20-res.yml	`5db46f5eca4bb180...`	`531a6705ffd1cb07...`
bitcoin-core-osx-0.20-res.yml	`9db10d1794650fb3...`	`df8f5ee6051d739b...`
bitcoin-core-win-0.20-res.yml	`89078fed82c8928e...`	`3080c08906b966bf...`
linux-build.log	`a3067a2425690fd1...`	`5729d5af45be9425...`
osx-build.log	`a15a76ffd1535083...`	`92478cfb5c2b9455...`
win-build.log	`615dbd3684fcd0b0...`	`23ca0dee1ee3fd70...`
bitcoin-core-linux-0.20-res.yml.diff		`752f63b68fe13c99...`
bitcoin-core-osx-0.20-res.yml.diff		`8b1ae7821af4ca09...`
bitcoin-core-win-0.20-res.yml.diff		`64b1ae0e9f8f9cdd...`
linux-build.log.diff		`9b0f122c4c7126af...`
osx-build.log.diff		`d10aef9c012c9b44...`
win-build.log.diff		`881623b0ad491f2e...`

DrahtBot removed the label Needs gitian build on Jan 3, 2020

practicalswift commented at 0:27 am on January 6, 2020: contributor

Concept ACK

Thanks for removing all this old cruft.

DrahtBot commented at 11:20 pm on January 22, 2020: member

DrahtBot added the label Needs rebase on Jan 22, 2020

build: remove configure checks for win libraries we don't link against

While cross compiling, HOST=x86_64-w64-mingw32, none of these
libs actually seem to be passed to the linker.

2525c096b0

fanquake force-pushed on Jan 23, 2020

fanquake requested review from sipsorcery on Jan 23, 2020

fanquake removed the label Needs rebase on Jan 23, 2020

sipsorcery approved

sipsorcery commented at 9:01 am on January 23, 2020: member

ACK 2525c096b002a89d4c561e1474800496ad8ebd7e.

practicalswift commented at 11:36 am on January 23, 2020: contributor

ACK 2525c096b002a89d4c561e1474800496ad8ebd7e – diff looks correct

fanquake referenced this in commit 28fbe68fdc on Jan 24, 2020

fanquake merged this on Jan 24, 2020

fanquake closed this on Jan 24, 2020

fanquake deleted the branch on Jan 24, 2020

sidhujag referenced this in commit ed7cf3613c on Jan 24, 2020

luke-jr commented at 1:11 am on March 25, 2020: member

These checks don’t cause linkage. They just check whether the library exists.

This is incorrect. AC_CHECK_LIB does cause linkage…

I don’t see how this didn’t just reintroduce CVE-2012-1910.

Yuthana826 commented at 2:41 pm on April 7, 2020: none

https://github.com/btcsuite/btcutil/pull/161

luke-jr referenced this in commit ea45dc9f41 on Jun 9, 2020

sidhujag referenced this in commit 147434c785 on Nov 10, 2020

luke-jr referenced this in commit f781dd3970 on Nov 17, 2020

luke-jr referenced this in commit 1da857b250 on Nov 25, 2020

zkbot referenced this in commit 372f695d4d on Jun 5, 2021

luke-jr referenced this in commit ed4e035bc8 on Oct 6, 2021

DrahtBot locked this on Feb 15, 2022

build: remove configure checks for win libraries we don’t link against #17740

Conflicts

Gitian builds