[
https://issues.apache.org/jira/browse/THRIFT-6191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113551#comment-18113551
]
Jens Geyer commented on THRIFT-6191:
------------------------------------
h2. Direction decided, and a correction to the analysis above
h3. The framing above is not quite right
The description says the client "kept the flag", which reads as an oversight.
It was not.
{{9b9567b23}} touched {{TSocket.cpp}} as well (+6/-2) and made a *deliberately
different choice
per side*:
* server: drop {{AI_ADDRCONFIG}} unconditionally;
* client: keep it, and extend the previously Windows-only retry-without-it to
POSIX on
{{EAI_NODATA}}.
So the author saw both sides in the same commit and kept the flag on the client
on purpose --
presumably to avoid futile IPv6 connect attempts. THRIFT-5880 only widened that
retry with
{{EAI_ADDRFAMILY}}. Any direction that strips {{AI_ADDRCONFIG}} from the client
is reversing a
considered decision rather than fixing a slip.
h3. Measured, rather than reasoned about
A probe in a container with {{::1}} on {{lo}} and no other IPv6 address:
{code}
--- 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 + AI_ADDRCONFIG -> 127.0.0.1
--- wildcard bind: NULL host + AI_PASSIVE ---
server today (AI_PASSIVE|AI_V4MAPPED) -> 0.0.0.0 ::
server + AI_ADDRCONFIG -> 0.0.0.0
--- numeric ::1 ---
server today (AI_PASSIVE|AI_V4MAPPED) -> ::1
server + AI_ADDRCONFIG -> ERROR EAI_ADDRFAMILY (-9)
{code}
Two things this settles:
* {{AI_V4MAPPED}} is *decorative* on the server today.
{{AddressResolutionHelper::query()}} sets
{{ai_family = AF_UNSPEC}}, and {{AI_V4MAPPED}} is ignored unless the family
is {{AF_INET6}}.
The entire behavioural difference between the two sides is {{AI_ADDRCONFIG}}.
* The wildcard bind is *unaffected*. {{0.0.0.0}} already sorts ahead of {{::}},
so the server
binds IPv4 either way; and where IPv6 genuinely is configured,
{{AI_ADDRCONFIG}} does not
remove {{::}}.
h3. The three directions in the description
* *bind {{::}} instead of the first resolved address* -- rejected. For the
wildcard case this is
already what happens. For a named address it is a silent and
security-relevant behaviour
change: {{TServerSocket("127.0.0.1", 9090)}} is how a caller keeps a server
off the network,
and this would turn it into a bind on every interface.
* *try every resolved address* -- does not fix this, because {{::1}} *binds
successfully*; the
iteration never triggers. Read as "bind them all" it means a multi-socket
listener, which is a
much larger change to {{TServerSocket}}. Worth noting separately that the
existing loop does
not do what its comment claims -- see the follow-up below.
* *drop {{AI_ADDRCONFIG}} on the client* -- works, but reverses the 5186
decision and
reintroduces what the flag exists to prevent: on a v4-only network talking to
a dual-stack
remote, the client tries the AAAA first, which on a network that blackholes
IPv6 blocks until
the connect timeout.
h3. A fourth direction, which is what was implemented
Make the *server* match the *client*, not the other way round: resolve with
{{AI_ADDRCONFIG}} and
fall back to resolving without it when that leaves nothing to bind -- exactly
the shape
{{TSocket::open()}} already has.
* the container case: both sides see {{127.0.0.1}}, agree, connect;
* THRIFT-5186's case (host with no configured address): first resolve fails,
fallback resolves;
* THRIFT-5880's numeric case: covered by the same fallback -- and as the
measurement shows, an
explicit {{"::1"}} really does fail with {{EAI_ADDRFAMILY}}, so the fallback
is load-bearing
rather than defensive;
* no client-side connect-timeout hazard, because the client does not change;
* it is already shipping: the {{#ifdef ANDROID}} branch passed
{{AI_PASSIVE|AI_ADDRCONFIG}}
today, so Android never had this bug. That branch is now redundant and goes
away.
PR: [#3836|https://github.com/apache/thrift/pull/3836]
h3. Tests
{{TServerSocketTest/test_bind_to_address}} already covered this for
{{TServerSocket}}.
{{TNonblockingServerSocket}} carried the identical line with no equivalent
coverage, so
{{TNonblockingServerTest/bind_and_connect_agree_on_hostname}} was added. Both
were confirmed to
fail against the unmodified library first, with {{connect() failed: Connection
refused}}. Full
C++ suite in the affected container: 39/39 {{ctest}} pass, where {{UnitTests}}
was red before.
h3. Two follow-ups found while checking direction 2, both separate from this
ticket
* {{TServerSocket::listen()}}'s bind loop is commented "we iterate over what
system gave us,
picking the first address that works", but {{retryLimit_}} defaults to *0*, so
{{while ((retries++ < retryLimit_) && ...)}} is false after the first
address. Only one address
is ever tried unless the caller sets {{setRetryLimit()}}. A {{socket()}}
failure for {{AF_INET6}}
on a kernel without IPv6 {{continue}}s into the same false condition instead
of falling through
to the IPv4 address.
* the same loop leaks a descriptor: when {{retryLimit_ > 0}} and {{bind()}}
fails, the next
iteration overwrites {{serverSocket_}} with a fresh {{socket()}} without
closing the previous one.
Also noted, harmless today: {{AddressResolutionHelper}} defaults its flags to
{{AI_V4MAPPED | AI_ADDRCONFIG}} with no {{AI_PASSIVE}} -- a third combination
that agrees with
neither call site. Both existing callers pass explicit flags, so it is
unreachable, but it is a
trap for the next one.
_Analysis and patch prepared with AI assistance (Claude Opus 5); reviewed and
submitted by Jens Geyer._
> C++ server sockets and TSocket resolve the same host with different
> getaddrinfo flags
> -------------------------------------------------------------------------------------
>
> Key: THRIFT-6191
> URL: https://issues.apache.org/jira/browse/THRIFT-6191
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
> Reporter: Jens Geyer
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> h2. What happens
> {{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}}.
> On a host where {{/etc/hosts}} lists {{::1 localhost}} but no IPv6 address is
> configured on
> {{lo}} -- a default Docker container, for instance -- the two disagree about
> what
> {{"localhost"}} means:
> ||Caller||Flags||First address returned||
> |{{TServerSocket::listen()}}|{{AI_PASSIVE}} + {{AI_V4MAPPED}}|{{::1}}, and
> the bind succeeds|
> |{{TSocket::open()}}|{{AI_PASSIVE}} + {{AI_ADDRCONFIG}}|{{127.0.0.1}}|
> A C++ server bound to {{"localhost"}} therefore listens on {{::1}} while a
> C++ client connecting
> to {{"localhost"}} on the same machine dials {{127.0.0.1}}, and the connect
> fails with
> {{ECONNREFUSED}}.
> The {{IPV6_V6ONLY=0}} that {{listen()}} sets on an {{AF_INET6}} socket does
> not rescue it: the
> bind is to {{::1}} specifically rather than to {{::}}, and a v4-mapped
> address is not covered by
> that.
> h2. Where it shows
> {{lib/cpp/test/TServerSocketTest.cpp}}'s {{test_bind_to_address}} fails in
> exactly this
> environment, and has nothing to do with the test itself:
> {noformat}
> unknown location(0): fatal error: in "TServerSocketTest/test_bind_to_address":
> apache::thrift::transport::TTransportException: connect() failed: Connection
> refused
> {noformat}
> CI does not catch it. GitHub runners have {{::1}} on {{lo}}, so
> {{AI_ADDRCONFIG}} returns IPv6
> for the client as well and both sides agree again.
> h2. Measured
> A standalone probe that resolves and binds with each flag set and then
> connects, run in a
> container with no IPv6 address on {{lo}}:
> {noformat}
> server pre-0.14.0 (AI_PASSIVE|AI_ADDRCONFIG) server binds IPv4, client
> tries IPv4 -> CONNECTED
> server today (AI_PASSIVE|AI_V4MAPPED) server binds IPv6, client
> tries IPv4 -> Connection refused
> server today, without IPV6_V6ONLY=0 server binds IPv6, client
> tries IPv4 -> Connection refused
> {noformat}
> h2. When it changed
> Last worked in 0.13.0. THRIFT-5186 removed {{AI_ADDRCONFIG}} from the server
> sockets in
> {{9b9567b23}} (2020-04-27), first released in 0.14.0. That change was right
> on its own terms --
> {{AI_ADDRCONFIG}} does not count the loopback address as a configured
> address, so a host with no
> other address could not resolve localhost at all. The client side kept the
> flag, and the
> asymmetry is what breaks.
> THRIFT-5880 ({{25202e1b0}}, 0.23.0) later gave {{TSocket::open()}} a retry
> without
> {{AI_ADDRCONFIG}}, but it is conditional on {{EAI_NODATA}} /
> {{EAI_ADDRFAMILY}} and does not fire
> here, because the first call succeeds.
> h2. Possible directions
> Not decided, and worth discussing before anyone writes a patch:
> * bind {{::}} rather than the first resolved address, keeping
> {{IPV6_V6ONLY=0}};
> * try every resolved address instead of stopping at the first that binds;
> * drop {{AI_ADDRCONFIG}} on the client too, matching what THRIFT-5186 did for
> the server.
> {{TServerSocket}} and {{TNonblockingServerSocket}} carry the same line and
> would need the same
> treatment.
> _Filed with AI assistance (Claude Opus 5); reviewed and filed by Jens Geyer._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
