Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug that allowed IP V6 addresses to be resolved instead of IP V4. #3184

Closed
wants to merge 1 commit into from

Conversation

tzachi-dar
Copy link
Contributor

No description provided.

@jamorham
Copy link
Collaborator

You need to provide a description of the problem and resolution in the PR description.

Is an Ipv6 address always incorrect? How can we tell if people are actually intentionally using ipv6 in this situation?

Also can you use host.getAddress().length == 4 instead of adding a new function to parse the host string?

@tzachi-dar
Copy link
Contributor Author

So, the problem is that on my machines some of the host have been getting both ip-v6 addresses as well as ip-v4 addresses.
Since many parts of our software don't work with ipv6 I assumed this was a bug.

Also looking at a lines like:
"Skipping overwrite of " + short_name + " with " + address + " due to ipv4 priority"
I was under the impression that we always want to use ipv4 addresses, but that might be wrong. I guess that if a system is only ip v6 then only ip v6 addresses will be used.

The problem is that if there are both ip v4 and ip v6 addresses we are not guaranteed to always get a result for the ip v4. In that case for 10 minutes ip v6 will be used (which means no connection).

Here is an example log of such a case:

C:\Users\nirit\take4\xDrip>findstr /i zero-test log2.txt | findstr getAddrInfo
11-15 02:52:02.978   619   731 D MDnsDS  : getAddrInfo(10, (null) 0, zero-test.local.)
11-15 02:52:02.978   619   730 D MDnsDS  : getAddrInfo succeeded for 10: 10 "zero-test.local." 120 fe80::63f9:cca0:c79a:25d0
11-15 03:02:07.862   619   731 D MDnsDS  : getAddrInfo(28, (null) 0, zero-test.local.)
11-15 03:02:07.863   619   730 D MDnsDS  : getAddrInfo succeeded for 28: 28 "zero-test.local." 120 192.168.4.7
11-15 03:02:07.863   619   730 D MDnsDS  : getAddrInfo succeeded for 28: 28 "zero-test.local." 120 fe80::63f9:cca0:c79a:25d0

So, if we want to give ipv6 a priority, then I think that we should have another variable that will check if some hosts have received an ipv4 addresses, and if they have then ipv6 should be ignored in that network (for an hour?).

Another approach is to do that for each IP (check if it was once an ipv4 addresses and then don't allow it to become ipv6).

If you have other ideas on how to implement that I'll be happy to hear.

@jamorham
Copy link
Collaborator

So it sounds like the remote devices are only listening on ipv4 even though they may have both ipv4 and ipv6 connectivity.
I think limiting to ipv4 would be okay but I think we need to provide a preference switch to allow either for networks which support it. ipv6 is self configuring and does make sense for the peer to peer model being used here.

I would say add an option for allowing ipv6 address and have it disabled by default and then use the ip address filtering unless that preference option has been enabled. I think more people are likely to be having problems with unusable ipv6 than will actually be using ipv6 so I think it makes sense to default it like that.

Also please consider my suggestion for checking the address length by size rather than by a string filter which might be less reliable - for example in different locales (although unlikely I think its cleaner not to rely on the string)

@tzachi-dar
Copy link
Contributor Author

I'm changing the recievers to support IPV6 as well.
So, should I still keep this PR (with a switch) or just do the needed fixes in xDrip to support IPV6?

@jamorham
Copy link
Collaborator

jamorham commented Dec 5, 2023

I don't really know enough about the receivers to know the answer to that. How are things looking?

@tzachi-dar
Copy link
Contributor Author

This was replaced by #3229.

@tzachi-dar tzachi-dar closed this Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants