build: miniupnpc 2.2.2 #20421

fanquake · 2020-11-19T03:11:53Z

Creating the dll subdir is no-longer required.
We can drop our wingen patch.

One issue that came up in #19867:

unrelated to this change but: why are we inserting the architecture in here, seems like something not necessary to reveal

My assumption is that it was being inserted to make depends more deterministic. However I think we can improve this, as there's no reason to reveal more of the version information either. Could leave the version as is /2.0 and either drop the architecture, or insert something else?

I've dropped our sed and added a patch that just removes the OS string and miniupnpc version from the User-Agent. i.e:

# master
strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent
User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203
User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203

# this PR
strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent
User-Agent: UPnP/1.1
User-Agent: UPnP/1.1

Note that built unmodified (miniupnp/miniupnp@22c1386), the User-Agent would be:

strings libminiupnpc.dylib | rg User-Agent                                     
User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0
User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0

fanquake · 2020-11-19T04:55:59Z

This is currently failing on just the macOS cross build because it has -Werror enabled, which means means-Werror=gnu, and -Wzero-length-array is included in -Wgnu:

/usr/bin/ccache /tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/native/bin/clang++ -target x86_64-apple-darwin16 -mmacosx-version-min=10.12 --sysroot /tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers -stdlib=libc++ -mlinker-version=530 -B/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/native/bin -nostdinc++ -isystem /tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include/c++/v1 -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I.  -DBOOST_SP_USE_STD_ATOMIC -DBOOST_AC_USE_STD_ATOMIC -pthread -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/share/../include -I./leveldb/include -I./leveldb/helpers/memenv -I./secp256k1/include -I./univalue/include  -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/include -D_THREAD_SAFE -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/include -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/share/../include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0  -Wstack-protector -fstack-protector-all -fcf-protection=full   -Werror=gnu -Werror=vla -Werror=shadow-field -Werror=switch -Werror=thread-safety -Werror=range-loop-analysis -Werror=unused-variable -Werror=date-time -Werror=return-type -Werror=conditional-uninitialized -Werror=sign-compare -Werror=unreachable-code-loop-increment    -pipe -O2  -fvisibility=hidden -c -o interfaces/libbitcoin_server_a-chain.o `test -f 'interfaces/chain.cpp' || echo './'`interfaces/chain.cpp
/usr/bin/ccache /tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/native/bin/clang++ -target x86_64-apple-darwin16 -mmacosx-version-min=10.12 --sysroot /tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers -stdlib=libc++ -mlinker-version=530 -B/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/native/bin -nostdinc++ -isystem /tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include/c++/v1 -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I.  -DBOOST_SP_USE_STD_ATOMIC -DBOOST_AC_USE_STD_ATOMIC -pthread -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/share/../include -I./leveldb/include -I./leveldb/helpers/memenv -I./secp256k1/include -I./univalue/include  -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/include -D_THREAD_SAFE -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/include -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/share/../include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0  -Wstack-protector -fstack-protector-all -fcf-protection=full   -Werror=gnu -Werror=vla -Werror=shadow-field -Werror=switch -Werror=thread-safety -Werror=range-loop-analysis -Werror=unused-variable -Werror=date-time -Werror=return-type -Werror=conditional-uninitialized -Werror=sign-compare -Werror=unreachable-code-loop-increment    -pipe -O2  -fvisibility=hidden -c -o interfaces/libbitcoin_server_a-node.o `test -f 'interfaces/node.cpp' || echo './'`interfaces/node.cpp
/usr/bin/ccache /tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/native/bin/clang++ -target x86_64-apple-darwin16 -mmacosx-version-min=10.12 --sysroot /tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers -stdlib=libc++ -mlinker-version=530 -B/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/native/bin -nostdinc++ -isystem /tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include/c++/v1 -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I.  -DBOOST_SP_USE_STD_ATOMIC -DBOOST_AC_USE_STD_ATOMIC -pthread -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/share/../include -I./leveldb/include -I./leveldb/helpers/memenv -I./secp256k1/include -I./univalue/include  -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/include -D_THREAD_SAFE -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/include -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/share/../include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0  -Wstack-protector -fstack-protector-all -fcf-protection=full   -Werror=gnu -Werror=vla -Werror=shadow-field -Werror=switch -Werror=thread-safety -Werror=range-loop-analysis -Werror=unused-variable -Werror=date-time -Werror=return-type -Werror=conditional-uninitialized -Werror=sign-compare -Werror=unreachable-code-loop-increment    -pipe -O2  -fvisibility=hidden -c -o libbitcoin_server_a-net.o `test -f 'net.cpp' || echo './'`net.cpp
In file included from net.cpp:36:
In file included from /tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/share/../include/miniupnpc/miniupnpc.h:14:
/tmp/cirrus-ci-build/depends/x86_64-apple-darwin16/share/../include/miniupnpc/upnpdev.h:27:14: error: zero size arrays are an extension [-Werror,-Wzero-length-array]
        char buffer[0];
                    ^
1 error generated.
Makefile:11312: recipe for target 'libbitcoin_server_a-net.o' failed
make[2]: *** [libbitcoin_server_a-net.o] Error 1

Note that the ARM CI, which also has -Werror, and uses depends, is not currently failing. ~~I assume because __GNUC__ is not defined.~~ The problem code:

#if defined(__STDC_VERSION) && __STDC_VERSION__ >= 199901L
	/* C99 flexible array member */
	char buffer[];
#elif defined(__GNUC__)
	char buffer[0];
#else
	/* Fallback to a hack */
	char buffer[1];
#endif

was introduced in miniupnpc in miniupnp/miniupnp@47a55b2.

laanwj · 2020-11-19T09:28:47Z

depends/patches/miniupnpc/dont_leak_info.patch

+ 			     "Host: %s:%d\r\n"
+ 				 "Connection: Close\r\n"
+-				 "User-Agent: " OS_STRING ", " UPNP_VERSION_STRING ", MiniUPnPc/" MINIUPNPC_VERSION_STRING "\r\n"
+				 "User-Agent: " UPNP_VERSION_STRING "\r\n"


Nice, thanks for doing this.

Can we upstream this as a public API option?

hebasto · 2020-11-19T09:45:16Z

Concept ACK.

practicalswift · 2020-11-19T10:54:36Z

Concept ACK

Thanks for taking care of the version leaking. I've never understood why miniupnpc choose to leak verbose version info like this: it reduces the cost of successful attack a lot.

laanwj · 2020-11-23T14:54:26Z

The problem is that there is no legal way to do this in C++ at all, besides through the GNU extension. The [1] hack is all you can get away with in any standard, and it might run into issues with static and dynamic checkers.

If you want that, you can apparently stop clang from defining __GNUC__ with -fgnuc-version=0. That said, passing -Wno-zero-length-array might be more reasonable.

Note that the ARM CI, which also has -Werror, and uses depends, is not currently failing. I assume because GNUC is not defined.

I suppose that's because the ARM builds use gcc, not clang, which doesn't have that warning.

luke-jr · 2020-11-24T23:57:58Z

Concept ACK

hebasto

Approach ACK 4cc41b3.

I think CI will pass after #20182 (https://cirrus-ci.com/build/6495395879583744).

Maybe rebase? And, probably, bump version to 2.2.2?

Creating the dll subdir is no-longer required. We can also drop our wingen patch.

fanquake · 2021-03-23T00:49:01Z

I think CI will pass after #20182 (https://cirrus-ci.com/build/6495395879583744).

I'm not sure why that would make a difference? The same code causing the -Wzero-length-array issue is still in the upnpdev.h header, and we're still building with --enable-werror:

In file included from mapport.cpp:25:
In file included from bitcoin/depends/x86_64-apple-darwin19.6.0/include/miniupnpc/miniupnpc.h:14:
bitcoin/depends/x86_64-apple-darwin19.6.0/include/miniupnpc/upnpdev.h:27:14: error: zero size arrays are an extension [-Werror,-Wzero-length-array]
        char buffer[0];
                    ^
1 error generated.

In any case, I've rebased, and updated this branch to version 2.2.2. Will just look at patching out the problematic code.

hebasto · 2021-03-23T02:06:50Z

... and we're still building with --enable-werror

Right. Plus --enable-suppress-external-warnings :)

fanquake · 2021-03-23T02:08:33Z

Right. Plus --enable-suppress-external-warnings :)

Sure, in the CI. However that's not useful for regular users who just want to build with --enable-werror. Seems like you're suggesting that --enable-werror is going to become useless without --enable-suppress-external-warnings? That's not really a path I want to go down. I don't like the idea of just suppressing everything for the sake of a warning-less build.

hebasto · 2021-03-23T02:10:58Z

Right. Plus --enable-suppress-external-warnings :)

Sure, in the CI. However that's not useful for regular users who just want to build with --enable-werror. Seems like you're suggesting that --enable-werror is going to become useless without --enable-suppress-external-warnings?

That was the point to introduce the latter, no?

hebasto

ACK 180dc3c.

Gitian builds:

Linux:

Generating report
ceecd19802fe907d38a2ddda188feb42b5cb6c0c928242481beb8540f83c6028  bitcoin-180dc3c8863f-aarch64-linux-gnu-debug.tar.gz
4e8bab9604433f104ac28db02be353fe14665abbe38056950022fbbdd4a99148  bitcoin-180dc3c8863f-aarch64-linux-gnu.tar.gz
1ccf71e01c5c547d4f1e104735814851ab7e6c0eddf83b56f287a7269641a561  bitcoin-180dc3c8863f-arm-linux-gnueabihf-debug.tar.gz
c817879efad5439806207f69b189f41025a3a2ee85f24d5b561836252d555ef8  bitcoin-180dc3c8863f-arm-linux-gnueabihf.tar.gz
ac233f5ad64369ccbdad2bd7ab19d0510f1a2fa1a6e2e4cd1adab8c6ad94fd8a  bitcoin-180dc3c8863f-powerpc64-linux-gnu-debug.tar.gz
78884c8e636eeacb810912d1ab801cc061874641c388b6d561038f7d9dd05a34  bitcoin-180dc3c8863f-powerpc64-linux-gnu.tar.gz
1e159cb35511dbc937d7436bea68ab068f9bad80b44bb627d1b80e235f00f886  bitcoin-180dc3c8863f-powerpc64le-linux-gnu-debug.tar.gz
f1dca9e18336039384d944ca9c151c417e13b5faa9bb677d2005a7a959114319  bitcoin-180dc3c8863f-powerpc64le-linux-gnu.tar.gz
49dd9e7be46c15a00400bad53be3345756f1cba04879c63184bcea0a92d8c5ae  bitcoin-180dc3c8863f-riscv64-linux-gnu-debug.tar.gz
42b23d405d0badb18aa995b150b54e42d3511ea3f73f660734fcab99ba11e16e  bitcoin-180dc3c8863f-riscv64-linux-gnu.tar.gz
cb7f6547050629ade7afb73c50cd069819f110adbb52cb0ba1ca38bde266d4d1  bitcoin-180dc3c8863f-x86_64-linux-gnu-debug.tar.gz
a4266cf56175445544ec10c2349e2761676d326900d0674b5d3ffb782388be55  bitcoin-180dc3c8863f-x86_64-linux-gnu.tar.gz
9160e96fcd64634d4f33885241b6ab4b7884a4504431777b790b67e74e6fdc69  src/bitcoin-180dc3c8863f.tar.gz
3e9532e5fcb454a2356eadef4b7d0b32828df8ea29290d1cff1154e9b547b818  bitcoin-core-linux-22-res.yml
Done.

bitcoin-180dc3c8863f-x86_64-linux-gnu.tar.gz tested on Linux Mint 20.1

Windows:

Generating report
0df1d25cd591b12512faefbff9e5be2b51ce980e04ddd51b83cc35449b917fde  bitcoin-180dc3c8863f-win-unsigned.tar.gz
2ad623c3dad5e1f24b8e672bec97f3cb21e8bf0aa768f4be2762486f84cab064  bitcoin-180dc3c8863f-win64-debug.zip
f5e34a3ca32756303f2303a2153b999f7f51e8ea01e1713e4e1c8553b72c2aea  bitcoin-180dc3c8863f-win64-setup-unsigned.exe
4c1b4313875269aee017e0cc985b3bef8e880537d5fe80b0b858ffe85ca95a76  bitcoin-180dc3c8863f-win64.zip
9160e96fcd64634d4f33885241b6ab4b7884a4504431777b790b67e74e6fdc69  src/bitcoin-180dc3c8863f.tar.gz
115ea6a8a906ec25a8f1d63bfe41368e5ebaceb68802aa1ba053c179495181c5  bitcoin-core-win-22-res.yml
Done.

bitcoin-180dc3c8863f-win64.zip tested on Windows 10 Pro 20H2 (build 19042.867)

macOS:

Generating report
f7825c2827390ebda9fd8853f90374927219924a2ef916f2d9ca6e219f2db3ab  bitcoin-180dc3c8863f-osx-unsigned.dmg
a88d9a0c95b87cdc3f07ffd85c2d3563333c8042218ef2a2fb467494dcf177de  bitcoin-180dc3c8863f-osx-unsigned.tar.gz
16f58d69cb717434fda1cae05a8f1dbc9b63aaff32f197bf7b1d418a267a1140  bitcoin-180dc3c8863f-osx64.tar.gz
9160e96fcd64634d4f33885241b6ab4b7884a4504431777b790b67e74e6fdc69  src/bitcoin-180dc3c8863f.tar.gz
fa1bed6220152c9cf5b67f4b84b09245dd0818cc80bfffd10c8fc50c554a93ea  bitcoin-core-osx-22-res.yml
Done.

bitcoin-180dc3c8863f-osx-unsigned.dmg tested on macOS BigSur 11.2.3 (20D91)

A router powered by OpenWrt was used for testing UPnP functionality.

laanwj · 2021-03-23T19:34:56Z

I've done a gitian build and my hashes exactly match @hebasto's
Code review ACK 180dc3c

180dc3c build: miniupnpc 2.2.2 (fanquake) Pull request description: Creating the dll subdir is no-longer required. We can drop our wingen patch. One issue that came up in [bitcoin#19867](bitcoin#19867 (comment)): > unrelated to this change but: why are we inserting the architecture in here, seems like something not necessary to reveal > My assumption is that it was being inserted to make depends more deterministic. However I think we can improve this, as there's no reason to reveal more of the version information either. Could leave the version as is /2.0 and either drop the architecture, or insert something else? I've dropped our `sed` and added a patch that just removes the OS string and miniupnpc version from the User-Agent. i.e: ```bash # master strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203 User-Agent: x86_64-apple-darwin19.6.0, UPnP/1.1, MiniUPnPc/2.0.20180203 # this PR strings depends/x86_64-apple-darwin19.6.0/lib/libminiupnpc.a | rg -i User-Agent User-Agent: UPnP/1.1 User-Agent: UPnP/1.1 ``` Note that built unmodified (miniupnp/miniupnp@22c1386), the User-Agent would be: ```bash strings libminiupnpc.dylib | rg User-Agent User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0 User-Agent: Darwin/19.6.0, UPnP/1.1, MiniUPnPc/2.2.0 ``` ACKs for top commit: laanwj: Code review ACK 180dc3c hebasto: ACK 180dc3c. Tree-SHA512: b0b6e623dbc5499e28faedf992d84278d6a11887a45a3806957b9e08886c5e56044cdfa2e7d7ec81cb1dd55f89be99834367905315d6bc611ba530e91d889ad1

2e9c487 build: miniupnpc 2.2.2 (fanquake) Pull request description: Coming from bitcoin#20421 Creating the dll subdir is no-longer required. We can also drop our wingen patch. ACKs for top commit: furszy: ACK 2e9c487 random-zebra: utACK 2e9c487 and merging... Tree-SHA512: 96f476a5e928a5ac7abb110d7573817be4c878fc5570f0c8a83acd53ba277bc4b0858bfa1f849324e7ae02525eaa77fd081ae83f80d09d5f7289483cbf8fb077

Security Ref: bitcoin/bitcoin#20421

fanquake added the Build system label Nov 19, 2020

fanquake added this to the 22.0 milestone Nov 19, 2020

laanwj reviewed Nov 19, 2020

View reviewed changes

laanwj added the Upstream label Nov 23, 2020

hebasto reviewed Mar 22, 2021

View reviewed changes

build: miniupnpc 2.2.2

180dc3c

Creating the dll subdir is no-longer required. We can also drop our wingen patch.

fanquake changed the title ~~build: miniupnpc 2.2.0~~ build: miniupnpc 2.2.2 Mar 23, 2021

fanquake force-pushed the miniupnpc_220 branch from 4cc41b3 to 180dc3c Compare March 23, 2021 00:49

hebasto approved these changes Mar 23, 2021

View reviewed changes

laanwj merged commit fa2a5b8 into bitcoin:master Mar 23, 2021

Fuzzbawls mentioned this pull request Apr 22, 2021

[Depends] Bump miniupnpc to 2.2.2 PIVX-Project/PIVX#2329

Merged

fanquake deleted the miniupnpc_220 branch May 8, 2021 02:48

barton2526 added a commit to barton2526/Gridcoin-Research that referenced this pull request Jun 12, 2021

build: miniupnpc 2.2.2

71dac55

Security Ref: bitcoin/bitcoin#20421

barton2526 mentioned this pull request Jun 12, 2021

build: miniupnpc 2.2.2 gridcoin-community/Gridcoin-Research#2179

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: miniupnpc 2.2.2 #20421

build: miniupnpc 2.2.2 #20421

Uh oh!

fanquake commented Nov 19, 2020

Uh oh!

fanquake commented Nov 19, 2020 •

edited

Loading

Uh oh!

laanwj Nov 19, 2020

Uh oh!

luke-jr Nov 24, 2020

Uh oh!

hebasto commented Nov 19, 2020

Uh oh!

practicalswift commented Nov 19, 2020

Uh oh!

laanwj commented Nov 23, 2020

Uh oh!

luke-jr commented Nov 24, 2020

Uh oh!

hebasto left a comment •

edited

Loading

Uh oh!

fanquake commented Mar 23, 2021

Uh oh!

hebasto commented Mar 23, 2021

Uh oh!

fanquake commented Mar 23, 2021 •

edited

Loading

Uh oh!

hebasto commented Mar 23, 2021

Uh oh!

hebasto left a comment

Uh oh!

laanwj commented Mar 23, 2021

Uh oh!

Uh oh!

build: miniupnpc 2.2.2 #20421

build: miniupnpc 2.2.2 #20421

Uh oh!

Conversation

fanquake commented Nov 19, 2020

Uh oh!

fanquake commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 19, 2020

Uh oh!

practicalswift commented Nov 19, 2020

Uh oh!

laanwj commented Nov 23, 2020

Uh oh!

luke-jr commented Nov 24, 2020

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Mar 23, 2021

Uh oh!

hebasto commented Mar 23, 2021

Uh oh!

fanquake commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Mar 23, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 23, 2021

Uh oh!

Uh oh!

fanquake commented Nov 19, 2020 •

edited

Loading

hebasto left a comment •

edited

Loading

fanquake commented Mar 23, 2021 •

edited

Loading