linuxdcpp-team team mailing list archive

[eMTee <1979900@xxxxxxxxxxxxxxxxxx>]

[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