Skip to content

Add support for miniupnpc 2.2.8 #2453

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

mwarning
Copy link
Contributor

Some packages like for OpenWrt use the native miniupnpc library that is not API compatible with the one included in ZeroTier.

From OpenWrt downstream: openwrt/packages#26088

@CLAassistant
Copy link

CLAassistant commented Apr 15, 2025

CLA assistant check
All committers have signed the CLA.

Some packages like for OpenWrt use the native miniupnpc
library that is not API compatible with the one included
in ZeroTier.
@laduke laduke requested a review from joseph-henry April 15, 2025 14:35
@laduke
Copy link
Contributor

laduke commented Apr 15, 2025

thank you mwarning!
Do you happen to know what errors are caused by not having this patch is? Is there an issue somewhere on openwrt? Just curious.

@mwarning
Copy link
Contributor Author

This is what the original problem: openwrt/openwrt#18019
I am not actually sure this is the correct fix as compilation should have failed if the signature does not match.
But there were reports of the segfault beeing fixed now.

@glimberg
Copy link
Contributor

I'm curious how you're building it. miniupnp is part of our normal build process and statically linked into the binary. So unless you're doing something in the OpenWRT build process to remove that completely from the build and include paths, I can definitely see something funky happening. The actual results aren't what I would expect though 🤔

@mwarning mwarning changed the title Add support for support miniupnpc 2.2.8 Add support for miniupnpc 2.2.8 Apr 16, 2025
@mwarning
Copy link
Contributor Author

@glimberg yes,on OpenWrt we dynamicly link in the system miniupnpc library. It was originally done to save space to support 4mb flash devices. But after ZT uses more advanced C++ features and linking to uclibcxx was not possible anymore, this became less relevant, as libstdc++ needs to be linked now and it is rather huge compared to uclibcxx.

@jjm2473
Copy link

jjm2473 commented Jun 10, 2025

@mwarning @glimberg

I am not actually sure this is the correct fix as compilation should have failed if the signature does not match.
But there were reports of the segfault beeing fixed now.

I know how this happened.

  1. openwrt use include from non-system paths when cross-compiling. https://github.com/openwrt/packages/blob/019a8f1a98e14b8ba337d1dfae143d93ce121886/net/zerotier/patches/0001-fix-miniupnpc-natpmp-include-path.patch changes the MINIUPNPC_IS_NEW_ENOUGH detaction, use a miniupnpc/miniupnpc.h not in system include path. MINIUPNPC_IS_NEW_ENOUGH is 1 .
  2. zerotier make-linux.mk:12 contains INCLUDES?=-Irustybits/target -isystem ext ..., notice cflags -isystem ext
  3. zerotier osdep/PortMapper.cpp:47 contains #include <miniupnpc/miniupnpc.h>, cflags -isystem ext makes it actually include ext/miniupnpc/miniupnpc.h, because there is not miniupnpc/miniupnpc.h in default system include path (/usr/include, etc.).

This is the generated g++ command:

ccache aarch64-openwrt-linux-musl-g++ -Os -pipe -mcpu=generic -fno-caller-saves -fno-plt -fhonour-copts -ffile-prefix-map=/mnt/data/build/istoreos-build/24.10/armsr/openwrt/build_dir/target-aarch64_generic_musl/ZeroTierOne-1.14.1=ZeroTierOne-1.14.1 -ffunction-sections -fdata-sections -Wformat -Werror=format-security -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z,now -Wl,-z,relro -Wl,-z,noexecstack -ffunction-sections -fdata-sections  \
  -I/mnt/data/build/istoreos-build/24.10/armsr/openwrt/staging_dir/toolchain-aarch64_generic_gcc-13.3.0_musl/usr/include \
  -I/mnt/data/build/istoreos-build/24.10/armsr/openwrt/staging_dir/toolchain-aarch64_generic_gcc-13.3.0_musl/include -I/mnt/data/build/istoreos-build/24.10/armsr/openwrt/staging_dir/toolchain-aarch64_generic_gcc-13.3.0_musl/include/fortify  -Wall -Wno-deprecated -std=c++17 -pthread \
  -Irustybits/target \
  -isystem ext \
  -Iext/prometheus-cpp-lite-1.0/core/include -Iext-prometheus-cpp-lite-1.0/3rdparty/http-client-lite/include -Iext/prometheus-cpp-lite-1.0/simpleapi/include -DNDEBUG  -DZT_USE_MINIUPNPC \
  -DZT_USE_SYSTEM_MINIUPNPC \
  -DZT_USE_SYSTEM_NATPMP -DZT_NO_TYPE_PUNNING -DZT_ARCH_ARM_HAS_NEON -march=armv8-a+crypto -mtune=generic -mstrict-align -DZT_BUILD_PLATFORM=1 -DZT_BUILD_ARCHITECTURE=4 -DZT_SOFTWARE_UPDATE_DEFAULT="\"disable\"" -D_MT_ALLOCATOR_H -D_POOL_ALLOCATOR_H -D_EXTPTR_ALLOCATOR_H -D_DEBUG_ALLOCATOR_H   -c -o osdep/PortMapper.o osdep/PortMapper.cpp

-I/mnt/data/build/istoreos-build/24.10/armsr/openwrt/staging_dir/toolchain-aarch64_generic_gcc-13.3.0_musl/usr/include is normal include, not a system include. so #include <miniupnpc/miniupnpc.h> is found in -isystem ext.

zerotier use own ext/miniupnpc/miniupnpc.h when compiling, but link to system libminiupnpc when running.
that's why there are no errors when compiling, but it crashes when running.

Archlinux workaround this by simply rm -rf ext/miniupnpc/ https://gitlab.archlinux.org/archlinux/packaging/packages/zerotier-one/-/blob/main/PKGBUILD?ref_type=heads#L31

@Neustradamus
Copy link

Dear @zerotier members, @glimberg, @laduke, @joseph-henry: Any progress on this PR?

@glimberg
Copy link
Contributor

@Neustradamus We don't manage the build for OpenWRT. If someone comes up with a PR that doesn't break our build distributions and fixes issues over there, we'll happily merge it.

@BKPepe
Copy link

BKPepe commented Jun 11, 2025

@glimberg What about syncing and using an up-to-date miniupnc version instead of shipping an outdated bundled library? 🤔
It could be done as a submodule, and it also would be great if you could let users decide if they want to use their library as ArchLinux does or use your bundled library.

BTW: It is not about OpenWrt; it happens also on other GNU/Linux distros.

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

Successfully merging this pull request may close these issues.

7 participants