Use configured net interface even when it is missing

[ghost]

Follow up to #12253 (comment)

[ghost]

Just to add, qBt currently passes localhost to only the listen interfaces. Not for outgoing interface. So IP gets leaked anyway. The correct thing to do would be not force localhost on startup due to invalid interface name/IP. This way IP will not get leaked and user can continue to download as soon as VPN/Specific interface/IP is back online.

[FranciscoPombal]

Just to add, qBt currently passes localhost to only the listen interfaces. Not for outgoing interface. So IP gets leaked anyway. The correct thing to do would be not force localhost on startup due to invalid interface name/IP. This way IP will not get leaked and user can continue to download as soon as VPN/Specific interface/IP is back online.

Sorry, I don't get it, how does not forcing localhost prevent the IP from getting leaked? I'm also confused by the libtorrent docs (https://libtorrent.org/reference-Settings.html#outgoing_interfaces); they say: An empty string binds outgoing connections to INADDR_ANY and port 0 (i.e. let the OS decide)., but also Setting this to an empty string will disable binding of outgoing connections. @arvidn.

[arvidn]

@FranciscoPombal I recently tried to clarify that: https://github.com/arvidn/libtorrent/pull/4492/files
I'm open to suggestions of how to improve it further

[FranciscoPombal]

For reference, the new docs say:

An empty string will not bind TCP sockets to a device, and let the network stack assign the local address.

@An0n666 as long as everything works as expected, this is fine by me (I don't have the time to test this right now, sorry). But the more I look into the libtorrent code, the more I think that the interface handling code in qBittorrent needs a severe overhaul. As an example, it is not clear whether the user is configuring the listen interfaces or the outgoing interfaces, it is not possible to configure them separately, and there is not fine-grain control of multi-homed setups.

[arvidn]

I think it was a bit of a mistake to separate listen_interfaces and outgoing_interfaces in libtorrent. There are situations where they can't really be separated. uTP sockets for instance, they aren't affected by outgoing_interfaces, since they need to use an existing UDP socket, which will be associated with a "listen_interface".

I think it's reasonable to set outgoing_interfaces to the same ones as listen_interfaces (note that it can't be the same string though, since listen_interfaces expects ports too).

I'm envisioning a future where binding outgoing TCP sockets to one of the listen interfaces is a bool settings, not allowing independent lists of interfaces.

[ghost]

For reference, the new docs say:

An empty string will not bind TCP sockets to a device, and let the network stack assign the local address.

@An0n666 as long as everything works as expected, this is fine by me (I don't have the time to test this right now, sorry). But the more I look into the libtorrent code, the more I think that the interface handling code in qBittorrent needs a severe overhaul. As an example, it is not clear whether the user is configuring the listen interfaces or the outgoing interfaces, it is not possible to configure them separately, and there is not fine-grain control of multi-homed setups.

I could test but need a test build. Currently don't have my system where I have build tools for windows.

[FranciscoPombal]

@An0n666

I could test but need a test build. Currently don't have my system where I have build tools for windows.

Is there something about this PR that is specific to Windows that you want to test (just curious)? In either case, maybe @xavier2k6 can get you a build.

[xavier2k6]

@An0n666

I could test but need a test build. Currently don't have my system where I have build tools for windows.

Is there something about this PR that is specific to Windows that you want to test (just curious)? In either case, maybe @xavier2k6 can get you a build.

@An0n666 let me know what exact qBittorrent & libtorrent commits you want in a test build & will try to get you one when I can.

[ghost]

@An0n666

I could test but need a test build. Currently don't have my system where I have build tools for windows.

Is there something about this PR that is specific to Windows that you want to test (just curious)? In either case, maybe @xavier2k6 can get you a build.

@An0n666 let me know what exact qBittorrent & libtorrent commits you want in a test build & will try to get you one when I can.

PR rebased based on current master. So you can use this branch. The latest commits on RC_1_2 would do.

[xavier2k6]

PR rebased based on current master. So you can use this branch. The latest commits on RC_1_2 would do.

@An0n666
Sorry for the delay, link below:

qBittorrent commit 9855558 with libtorrent RC_1_2 commit ebc2bfc

[ghost]

Ok I've tested it.

Now when started with invalid interface, listen sockets are not being opened.
Trackers remain in not contacted state.
DHT nodes remain at 0.

However, outgoing connections are still functional. The peers that were saved in fastresume file instantly connected through main interface after resuming the torrent.

Connection icon was turned red saying qBt failed to listen on selected port(which is expected).

@arvidn
If I understand correctly, we have to pass an empty string to outgoing interfaces as well for it to not make any outgoing connections?

[arvidn]

If I understand correctly, we have to pass an empty string to outgoing interfaces as well for it to not make any outgoing connections?

It's the other way around. I'm sorry that this API is not very good (I'll try to improve it in future versions).

Outgoing TCP peer connections are not affected by the listen_interfaces setting. All other connections (that I can think of) are. And really, there is no reason that TCP connections shouldn't be, it's just historical, from before uTP was a thing.

So, listen_interfaces was originally meant to just affect the actual TCP listen sockets. With uTP, each interface needs a UDP socket as well, and since the UDP socket is used for DHT, uTP and DHT trackers, they all end up being tied to the listen_interfaces setting. So, the outgoing_interfaces is currently really only used to bind outgoing TCP peer connections.

As far as I can tell, the most reasonable behavior for a client is to set outgoing_interfaces to the same as listen_interfaces, at least if listen_interfaces is anything but the default (i.e. the unspecified address).

Doing that should make the behavior more uniform and reasonable. As far as I know, then TCP connections and uTP connections would behave the same.

In fact, I'm envisioning a future where the outgoing_interfaces option is deprecated in favor of a boolean setting called something like: bind_outgoing_tcp_connections, which will use the listen_interfaces to decide what to bind the sockets to.

[ghost]

Nevermind...My VPN was setup as PPTP on Windows..Changed to TAP-Windows Adaptor now and the issue with outgoing connections is gone.
IP is no longer leaking on startup. And just enabling VPN makes everything working again. No need to restart client.

[ghost]

This PR will require a small change for Windows users. When the specific interface GUID can’t be fetched, force listening on localhost for both incoming and outgoing interfaces.

[arvidn]

does the GUID change when the VPN goes down and comes back up again?
I was under the impression the scenario we're talking about is the VPN going away, at which point (presumably) qBT has already been configured with the adapter (GUID) to use.

[ghost]

No it wasn't changing. My VPN was setup as PPTP device. Which apparently vanishes if disconnected. So GUID can't be fetched. Anyways that was solved by using a VPN adapter.

Btw I found a bug in qBt where it was passing empty strings to outgoing_interfaces if GUID was not valid. This was causing IP leaks for Windows users.
Whereas in Linux there's no issue regarding GUID and the outgoing interface was being set properly for invalid interfaces.

[ghost]

PR updated and tested.

qBt will just pass whatever invalid interface user has chosen. If interface become available later on, libtorrent will open up sockets and connection will resume.

Addressed the IP leak issue. If the interface GUID can't be fetched (in Windows), we'll pass whatever interface name was set instead of the GUID. As passing empty strings to outgoing interface was causing IP leaks.

[FranciscoPombal]

@An0n666 since you removed the previous explanatory comment, I think you should add new ones, explaining the reasoning of why it's now done like this when using libtorrent 1.2.x

[ghost]

@An0n666 since you removed the previous explanatory comment, I think you should add new ones, explaining the reasoning of why it's now done like this when using libtorrent 1.2.x

I'm not exactly sure what happens when you pass an invalid interface in RC_1_1. The listen sockets shouldn't be opened anyway.
I've added an explanation. Hope that helps.

[ghost]

I looked into RC1_1 and found that it's exactly the same.

The IP leak issue was introduced by qBt actually.
I looked into some past commits

qBittorrent/src/base/bittorrent/session.cpp

Lines 1175 to 1177 in 4d00435

	settingsPack.set_str(lt::settings_pack::outgoing_interfaces, chosenIP.toStdString());
	LogMsg(tr("Could not get GUID of configured network interface. Binding to IP %1").arg(chosenIP)
	, Log::WARNING);

and found that previously qBt was passing the IP address to outgoing_interface if GUID was invalid. Meaning that 127.0.0.1 will be passed to outgoing_interface. But in recent commits that line of code was removed and an empty string was being passed to libtorrent for outgoing interfaces. Probably someone misread the libtorrent documentation and made that change.

[sledgehammer999]

Please don't merge until 22nd of April.
I see this PR and the previous ones and I would like to overhaul the whole thing. I am going to take a stab at it.
If you don't see any news from me until the 22nd then feel free to proceed with this one.

[sledgehammer999]

Nevermind. I read the 2 functions now, and I can't immediately see how I can better refactor them.
On another note: You should drop the if (outgoingInterfaces.isEmpty()) { codeblock in Session::configureNetworkInterfaces(). As far as I can tell it is totally useless now.

[sledgehammer999]

I am sorry for remembering this so late in the PR process, but the commit title is probably unsuitable.
I propose something like this: Use configured net interface even when it is missing

[FranciscoPombal]

@sledgehammer999 can you please release 4.2.4 soon? The recent connectivity fixes in qBIttorrent + libtorrent side fix a lot of problems that users keep reporting because 4.2.4 is not officially released yet. Plus there are a bunch of other important fixes, such as the auto disk cache fix.

Releasing 4.2.4 will enable us to close the existing proxy/udp threads (and other threads) which have a lot of information that's not relevant anymore, so we can focus on the issues that are not yet fixed.

[sledgehammer999]

@FranciscoPombal I have heard you the 1st time. As you probably know this PR is important too for v4.2.4.

[FranciscoPombal]

@sledgehammer999

@FranciscoPombal I have heard you the 1st time. As you probably know this PR is important too for v4.2.4.

Sorry for insisting, just wanted to make sure. This one is also ready and can be included in the release: #12568

[Ryrynz]

@FranciscoPombal I have heard you the 1st time. As you probably know this PR is important too for v4.2.4.

To be fair, you could reply :P

Was thinking it would be great if Github like Microsoft's Feedback Hub would find similar or related issues first before allowing the creation of a new issue. Maybe it's best just to ignore these duplicate issues and delete them after the new version has been released. Hopefully going forward 4.x can keep the issues to a minimum, this client has had it's fair share and it might've put people off. it's come a long way though, thanks to all involved. I'm starting to phase out uTorrent now :)

[glassez]

LGTM, but cannot really test it.
Seems it shouldn't affect the cases when there are no problems with configured interface.

[sledgehammer999]

@An0n666 thx.

Merging and preparing to release v4.2.4