THRIFT-6191: Resolve server bind addresses with the same flags TSocket uses

[Jens-G]

The defect

TServerSocket::listen() and TNonblockingServerSocket::listen() resolve the bind address with AI_PASSIVE|AI_V4MAPPED; TSocket::open() resolves the connect address with AI_PASSIVE|AI_ADDRCONFIG.

AI_ADDRCONFIG does not count a loopback address as a configured one. So on a host that carries ::1 on lo and has no other IPv6 address — a default Docker container, for instance — the two disagree about what "localhost" means:

Caller	Flags	First address
TServerSocket::listen()	AI_PASSIVE|AI_V4MAPPED	::1, and the bind succeeds
TSocket::open()	AI_PASSIVE|AI_ADDRCONFIG	127.0.0.1

A C++ server on "localhost" therefore listens on ::1 while a C++ client on "localhost" dials 127.0.0.1, and the connect fails with ECONNREFUSED. The IPV6_V6ONLY=0 the server sets does not rescue it: the bind is to ::1 specifically, not to ::.

Measured in such a container, resolving with each flag set:

--- named host: localhost ---
server today  (AI_PASSIVE|AI_V4MAPPED)         -> ::1 127.0.0.1
client today  (AI_PASSIVE|AI_ADDRCONFIG)       -> 127.0.0.1
server after  (AI_PASSIVE|AI_ADDRCONFIG)       -> 127.0.0.1

--- numeric ::1 ---
server today  (AI_PASSIVE|AI_V4MAPPED)         -> ::1
server after  (AI_PASSIVE|AI_ADDRCONFIG)       -> ERROR EAI_ADDRFAMILY   <- why the fallback exists
server fallback (AI_PASSIVE)                   -> ::1

When it changed

Last worked in 0.13.0. THRIFT-5186 (9b9567b23, first released in 0.14.0) removed AI_ADDRCONFIG from the server sockets so a host with no configured address could still resolve localhost — correct on its own terms. That same commit handled the client differently: it kept the flag on TSocket::open() and instead extended the Windows-only retry-without-it to POSIX. THRIFT-5880 (25202e1b0) later widened that retry to EAI_ADDRFAMILY, but it is conditional on the first resolution failing and does not fire here, because it succeeds.

CI never caught it: GitHub runners have ::1 on lo, so AI_ADDRCONFIG returns IPv6 for the client too and both sides agree again.

The change

Give the servers the flags and the shape TSocket::open() already has: resolve with exactly its AI_PASSIVE|AI_ADDRCONFIG, and fall back to AI_PASSIVE alone when that leaves nothing to bind. Both sides then agree, and THRIFT-5186's case still works via the fallback.

The fallback is load-bearing rather than defensive — as the measurement above shows, an explicit "::1" fails outright with EAI_ADDRFAMILY under AI_ADDRCONFIG on such a host.

That drops AI_V4MAPPED, deliberately. It does nothing for the AF_UNSPEC query the servers make — resolving with and without it gives the same addresses for localhost, ::1, 127.0.0.1 and the wildcard in the container above — and Android's bionic rejects it outright: its getaddrinfo() fails any flag outside AI_MASK with EAI_BADFLAGS before it looks at the family (getaddrinfo.c, netdb.h). That is what the #ifdef ANDROID branch from ab72ebe worked around. With the flags now identical on every platform the branch goes away, and Android resolves exactly as before and only gains the fallback.

AddressResolutionHelper's default flags carried the same AI_V4MAPPED. Nothing in the library relies on that default, but TSocketUtils.h is installed, so it now defaults to AI_ADDRCONFIG alone. TServerExceptionTest.cpp explained its fixed 127.0.0.1 with the servers' old flags; that comment is reworded.

With that, nothing in the C++ library uses AI_V4MAPPED any more, so the fallback for it goes as well: #3737 had the build define it as 0 where the platform does not declare it — OpenBSD has no AI_V4MAPPED at all — and the checks in configure.ac and build/cmake/ConfigureChecks.cmake and the define in build/cmake/config.h.in are removed. #3737 is not in a release yet, so no released thrift/config.h ever carried that define. On OpenBSD the flags that reach getaddrinfo() are the same as with #3737.

The wildcard bind is unaffected: with an empty address the resolver already returns 0.0.0.0 ahead of ::, and where IPv6 is configured AI_ADDRCONFIG does not remove it.

Tests

• TServerSocketTest/test_bind_to_address already covered this for TServerSocket and fails in such a container.
• TNonblockingServerSocket carried the identical line with no equivalent coverage; TNonblockingServerTest/bind_and_connect_agree_on_hostname adds it.

Both were confirmed to fail against the unmodified library before the fix, with connect() failed: Connection refused, and pass after it.

No CI job builds for Android, so bionic's flag check was emulated in the same container with an LD_PRELOAD getaddrinfo() that returns EAI_BADFLAGS for any flag outside its AI_MASK:

Under the emulated check	AI_V4MAPPED still passed	This change
TNonblockingServerTest	6 of 7 cases fail	pass
TServerExceptionTest (uses TServerSocket)	3 cases fail	pass

Each failure is the same one: both resolve attempts are rejected (getaddrinfo() -> -1; Bad value for ai_flags) and listen() throws Could not resolve host for server socket. With this change TServerSocketTest passes under the emulation too, and the only flags that reach getaddrinfo() are AI_PASSIVE|AI_ADDRCONFIG and, for the unresolvable address TServerSocketTest feeds it, the AI_PASSIVE fallback.

Full C++ suite in thrift:jammy, CMake Debug build: 58 of 58 ctest pass. One caveat: TSSLSocketMatchNameTest is flaky there with or without this change — it dies with SIGPIPE in 3 of 10 runs on master and 4 of 10 with it — so a single full run can come out 57 of 58. Changed lines are clang-format clean.

(On master that Debug build does not link UnitTests or TTransportFactoryConfigTest: TConfiguration::DEFAULT_MAX_MESSAGE_SIZE is ODR-used without an out-of-class definition. For this run a definition was linked in from outside the tree; it is not part of this change.)

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

[Jens-G]

Code review

Found 1 issue:

1. On Android, listen() now always throws TTransportException(NOT_OPEN) for non-Unix-domain sockets. With the #ifdef ANDROID branch removed, both the first resolve and the fallback pass AI_V4MAPPED, and bionic's getaddrinfo() rejects any flag outside AI_MASK (which does not include AI_V4MAPPED) with EAI_BADFLAGS before it looks at ai_family (getaddrinfo.c, netdb.h). So the commit message's "AI_V4MAPPED is ignored for AF_UNSPEC queries" does not hold on Android; this is the case ab72ebe (fix for android #2406) worked around. TSocket::open() never passes AI_V4MAPPED, so resolving with exactly its flags (AI_PASSIVE | AI_ADDRCONFIG, then AI_PASSIVE) would avoid this. (bug due to AI_PASSIVE | AI_V4MAPPED in both resolve calls in TServerSocket.cpp and TNonblockingServerSocket.cpp)

thrift/lib/cpp/src/thrift/transport/TServerSocket.cpp

Lines 456 to 462 in 00132b1

	try {
	resolved_addresses.resolve(address_, port_str, SOCK_STREAM,
	AI_PASSIVE | AI_V4MAPPED | AI_ADDRCONFIG);
	} catch (const std::system_error&) {
	try {
	resolved_addresses.resolve(address_, port_str, SOCK_STREAM, AI_PASSIVE | AI_V4MAPPED);
	} catch (const std::system_error& e) {

thrift/lib/cpp/src/thrift/transport/TNonblockingServerSocket.cpp

Lines 384 to 390 in 00132b1

	try {
	resolved_addresses.resolve(address_, port_str, SOCK_STREAM,
	AI_PASSIVE | AI_V4MAPPED | AI_ADDRCONFIG);
	} catch (const std::system_error&) {
	try {
	resolved_addresses.resolve(address_, port_str, SOCK_STREAM, AI_PASSIVE | AI_V4MAPPED);
	} catch (const std::system_error& e) {

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Jens-G]

Re the review above (#3836 (comment)): confirmed, and fixed in 0b4335f.

Both server sockets now resolve with exactly TSocket::open()'s flags — AI_PASSIVE|AI_ADDRCONFIG, then AI_PASSIVE — so AI_V4MAPPED is out of both attempts. On glibc that changes nothing for the AF_UNSPEC query: both flag sets give the same addresses for localhost, ::1, 127.0.0.1 and the wildcard. AddressResolutionHelper's default flags carried the same AI_V4MAPPED and now default to AI_ADDRCONFIG alone, and the comment in TServerExceptionTest.cpp that still described the old server flags is reworded.

Verified by emulating bionic's check with an LD_PRELOAD getaddrinfo() that returns EAI_BADFLAGS for any flag outside its AI_MASK: at 00132b1, TNonblockingServerTest (6 of 7 cases) and TServerExceptionTest (3 cases) fail under it with Bad value for ai_flags; with the fix they pass, and so does TServerSocketTest.

The first commit's message still gives the wrong reason for dropping the ANDROID branch ("AI_V4MAPPED is ignored for AF_UNSPEC queries"); the follow-up's message corrects it, and the PR description is updated to match.

[Jens-G]

@brad0 — a heads-up, since this touches what #3737 just fixed. This PR takes AI_V4MAPPED out of the C++ library altogether (Android's bionic rejects the flag at runtime, see the review above), so nothing uses the symbol any more, and b7b79f7 removes the OpenBSD fallback for it from configure.ac, build/cmake/ConfigureChecks.cmake and build/cmake/config.h.in again. On OpenBSD the flags passed to getaddrinfo() stay what they were with #3737, where AI_V4MAPPED was defined as 0.

I can't build on OpenBSD here — could you check that this branch still builds for you, with CMake and/or autotools?