[2022-09-13 17:50] <cologic> it's not necessarily a hub bug depending on how
their dual-stack connectivity is configured -- there might be configurations
which might create a synthetic IPv6 address and resolve it by essentially NAT
[2022-09-13 17:51] <eMTee> Okay but in contrast traditionally DC++ has a
philosophy that we don't care and do anything undefined if we're presented with
non-standard or errorous things from peers. It is actually the philosophy of
the ADC protocol as well.
[2022-09-13 17:52] <cologic> well that's partly around user input too, things
the user can control
[2022-09-13 17:52] <cologic> my concern is a bit higher when it's something
there's not otherwise an end-user workaround for
[2022-09-13 17:52] <eMTee> The syntax of the values sent through in protocol is
not checked, either.
[2022-09-13 17:55] <eMTee> ADC does not permit having both IP's specified. It
is a possible scenario, question is that is is a legitimate scenario? Hubs
usually set one IP in the INF, the one where the user connected from.
[2022-09-13 17:55] <eMTee> Also we are facing a non-standard functionality at
work here. What's happening is clearly because of HBRI, a currently
non-standard extension.
[2022-09-13 17:58] <eMTee> Do we want to correct a scenario what's caused by
non-standard things, since this scenario can also happen other ways, some of
them not necessarily breaks the standard. Is it useful to react to these cases?
[2022-09-13 17:59] <eMTee> If we do it though then DC++ becomes fully
compatible with an ADC hubsoftware that is still maintained.
[2022-09-13 17:59] <cologic> I was wondering about "+ return get(v6 ?
SettingsManager::INCOMING_CONNECTIONS6 : SettingsManager::INCOMING_CONNECTIONS)
!= SettingsManager::INCOMING_DISABLED;" -- how does this interact with the
"auto"/UPnP-type settings
[2022-09-13 18:00] <cologic> in terms of whether
SettingsManager::INCOMING_CONNECTIONS6 and/or
SettingsManager::INCOMING_CONNECTIONS is defined?
[2022-09-13 18:02] <eMTee> SettingsManager::INCOMING_CONNECTIONSx is set by
ConnectivityManager, either based on the settings autodetected (if auto conn
det is enabled) or based on the manual connectivity settings (if auto conn det
is disabled)
[2022-09-13 18:04] <eMTee> The auto detection sets INCOMING_CONNECTIONS6 to
disabled if connectivity not present or rather when only a local v6 IP is
available.
[2022-09-13 18:22] <cologic> does this issue occur in passive mode too? In
theory it shouldn't care what its IP addresses are, so it'd be a bit
conceptually strange for it to affect it that way
[2022-09-13 18:25] <eMTee> it happens on passive outgoing C-C connections and
active search respond outgoing connections. The two place, where the changes
proposed in the code.
[2022-09-13 18:28] <eMTee> basically if you have v4 connectivity only and the
I6 field is present, it will attempt to connect to the I6 value. Even if it
isn't an IP but a garbage, or a local IP.
[2022-09-13 18:45] <cologic> some kinds of garbage can't be filtered out
deterministically/reliably so in those cases sure, trust the IP from the hub.
Even a local IP from the hub might be correct or useful in some way. So I get
what you're saying about generally not doing a lot of validation/error
checking. But if the network interfaces available just do not include IPv6,
that's not guesswork anymore
[2022-09-13 18:46] <cologic> (e.g., a hub on a LAN could genuinely see a local
address)
[2022-09-13 18:46] <cologic> it's just when it creates a situation which cannot
possibly be useful
[2022-09-13 18:47] <cologic> in general, one might figure a hub is incentivized
to enable C-C connections, search results, etc to work, so I'm not worried
about incentive alignment
[2022-09-13 18:48] <eMTee> If the connectivity is set up correctly, manually,
for LAN usage then this change shouldn't break that.
[2022-09-13 18:48] <eMTee> Auto conn det has never been able to detect LAN only
usage so you have to set it up manually anyway.
[2022-09-13 18:48] <cologic> that's true, yeah, it's geared towards "external"
IP
[2022-09-13 18:48] <cologic> or externally routable IP
[2022-09-13 18:53] <eMTee> I also checked the instantiate/destroy of the three
managers involved and while the order is wrong in most cases, it's impossible
to call this piece of code with the changes until Client objects created /
after Client objects destroyed. Those events are well before/after
instantiating/destroying the 3 managers.
[2022-09-13 18:55] <eMTee> (UserInfo FastAlloc list is member of the Client
object)
[2022-09-13 18:55] <eMTee> (and Clients are created/destroyed at/after
connecting/closing hubs)
[2022-09-13 20:05:30] <cologic> "Do we want to correct a scenario what's caused
by non-standard things, since this scenario can also happen other ways, some of
them not necessarily breaks the standard." this is the main reason to respond
to it, of course
[2022-09-13 20:45:01] <cologic> oh, your "relevant parts of the code" are,
yeah, relevant. They explain it quite well
[2022-09-13 20:53:18] <cologic> IMO your patch is an ongoingly useful safeguard
for an absurd scenario. As you point out, the only
not-actually-garbage-but-maybe-looks-like-garbage version of an IPv6 address
one gets from this isn't handled well by the autoconfiguration, and it is
possible to trigger without HBRI, it just happens to trigger it more easily
[2022-09-13 20:54:38] <cologic> there is, yeah, a slight loss of principles,
and hiding of a hub bug, in this case, but that's not necessarily going to be
true
** Changed in: dcplusplus
Status: Confirmed => Fix Committed
--
You received this bug notification because you are a member of
Dcplusplus-team, which is subscribed to DC++.
https://bugs.launchpad.net/bugs/1979900
Title:
Outgoing connections are attempted on IPv6 on some cases when there's
no v6 connectivity
Status in DC++:
Fix Committed
Bug description:
[2022-06-17 17:13] <eMTee> Recently I was surprised seeing "::" in the IP
column in the hubframe userlist for all other DC++ users in each LuaDCH hubs
I'm in. I've never seen such thing before.
[2022-06-17 17:14] <eMTee> Actually the hub sends I6:: in BINF for all DC++
users but not for users of some other clients.
[2022-06-17 17:16] <eMTee> LuaDCH devs confirmed that it is a known issue and
they're working on it.
[2022-06-17 17:18] <eMTee> They did not tell the cause but I'm guessing it is
because DC++ doesn't support HBRI, an unofficial ADC extension developed by
maksis in AirDC++ and what they're using for managing hybrid v4/v6 connetions.
[2022-06-17 17:20] <eMTee> I checked why this zero IP value is displayed
instead of the usual v4 address in DC++. Turned out it's
User::Identity::getIp() which prefers returning IPv6 when both IP adresses are
available.
[2022-06-17 17:22] <eMTee> This a hub bug and since I see we accept whatever
the hub send in an ADC command param without checking for format/value so it
causes a visual disruption.
[2022-06-17 17:24] <eMTee> But. I checked where User::Identity::getIp() used
elsewhere. Actually it is used for getting the IP for outgoing connections in
ConnectionManager and ClientManager.
[2022-06-17 17:26] <eMTee> In this buggy case it would return the IPv6 value
for all outgoing connection attempts, even if you have IPv4 connectivity only.
That obviously breaks outgoing connections to those users for whom the hub
supplies a wrong I6 value. Still, this isn't much of a problem as it caused by
getting some nonsense from a buggy hub.
[2022-06-17 17:28] <eMTee> Sending zero IP value from a hub is indeed
nonsense but if the hub sends the valid IPv6 address of the user in I6 then
this will actually break your outgoing connections the same way in case you
have v4 connectivity only.
[2022-06-17 17:29] <eMTee> It breaks uploads, CCPM and sending udp search
responds.
[2022-06-17 17:34] <eMTee> Looks like getting IPv6 addresses in BINF from the
hub seems to be nonexistent thing even from hubs running in dual IP mode.
[2022-06-17 17:39] <eMTee> Relevant parts of the code are
https://sourceforge.net/p/dcplusplus/code/ci/93a0986b55c5b07038451fd336a30149076a21d1/tree/dcpp/User.cpp#l86
,
https://sourceforge.net/p/dcplusplus/code/ci/93a0986b55c5b07038451fd336a30149076a21d1/tree/dcpp/ConnectionManager.cpp#l410
,
https://sourceforge.net/p/dcplusplus/code/ci/93a0986b55c5b07038451fd336a30149076a21d1/tree/dcpp/ClientManager.cpp#l455
[2022-06-17 17:41] <eMTee> tl;dr is that we should not attempt to estabilish
an outgoing connection using a v6 IP address when we have only v4 connectivity.
[2022-06-18 08:49] <eMTee> Possible fix is to check for the current
connectivity status and choose the right IP before attempting the connection.
[2022-06-18 08:56] <eMTee> As we see IPv4 is still the go to connection
around the network I guess we must make sure such connections work properly.
This bug can be clearly triggered by receiving a valid ADC command parameter
from a hub.
See also https://bugs.launchpad.net/adchpp/+bug/309402 regarding all
the ipv6 issues. The above linked parts of the code suggest that plans
in https://bugs.launchpad.net/adchpp/+bug/309402/comments/23 has not
been implemented yet in DC++.
The attached patch fixes this issue by using info from
ConnectivityManager and forces IPv4 address/connection when there's no
IPv6 connectivity set/detected. Otherwise it does the same as before,
allowing User::Identity::getIp() to decide what protocol to be
used/preferred.
But the question is do we need to care of this at all or just rely on
what the hub sends... in other words is this kind of safeguard worth
or needed at all? Or it's useful for a temporary fix until one of the
few maintained ADC hubs fixes its bug?
To manage notifications about this bug go to:
https://bugs.launchpad.net/dcplusplus/+bug/1979900/+subscriptions
_______________________________________________
Mailing list: https://launchpad.net/~linuxdcpp-team
Post to : [email protected]
Unsubscribe : https://launchpad.net/~linuxdcpp-team
More help : https://help.launchpad.net/ListHelp
