Re: ws2_32: Update WS_get_local_ips to not return a bogus IP address.
On Sat, Nov 21, 2015 at 2:12 PM, Jamie Taylor <Jamie.Taylor(a)pobox.com> wrote:
There were many shortcomings in the previous behavior, but most importantly it resulted in the first IP address in the returned hostent structure being the magic loopback address in the default network configuration on Mac OS X. The new behavior returns all IP addresses from non-loopback network adapters, and ensures that the addresses from the adapter for the default route are at the front of the list. Testing confirmed this behavior on Mac OS X.
This will most likely fix https://bugs.winehq.org/show_bug.cgi?id=37271 and https://bugs.winehq.org/show_bug.cgi?id=22819 but I do not have the programs under test in those bugs, so I can't verify.
I also do not have convenient access to a linux machine with an equivalent network configuration (i.e., at least two interfaces with default routes) to test.
Hi, Jamie. Thanks for your effort, as you may have observed in the patches list your patch is marked as deferred because we are in code freeze for Wine 1.8 release. This code was touched a few times in attempts to fix this same issue you are addressing so maybe this time it will be the definitive fix =) I have a similar (not so robust) old patch that I used on FreeBSD (and also works in Linux), could you try it in OSX just out of curiosity, please? It has printf to help showing the results so a make test in the ws2_32/tests folder is enough to check if it works. About your patch I just would like to say that you should follow the surrounding code style, so the { goes in the next line. I tested it on Linux and it works for Heroes of Might & Magic 2 (https://bugs.winehq.org/show_bug.cgi?id=22819). Best wishes, Bruno
On Tue, Nov 24, 2015 at 08:30:13PM +0800, Bruno Jesus wrote:
Hi, Jamie. Thanks for your effort, as you may have observed in the patches list your patch is marked as deferred because we are in code freeze for Wine 1.8 release. This code was touched a few times in attempts to fix this same issue you are addressing so maybe this time it will be the definitive fix =)
Hello, Bruno. Thanks for your review. I think that a true definitive fix would include removing the magic loopback address completely, as it's obviously a hack (and not a value that would ever be returned by Windows). That seems like a much more invasive change.
I have a similar (not so robust) old patch that I used on FreeBSD (and also works in Linux), could you try it in OSX just out of curiosity, please? It has printf to help showing the results so a make test in the ws2_32/tests folder is enough to check if it works.
Yes, that patch works for me. The main difference is that it includes the magic loopback address in the returned list if there are any routes to a loopback interface in the forwarding table (actually, one copy of the magic loopback address per loopback interface, I think... but there's generally only one loopback interface). And it doesn't return all of the addresses from an interface (although I've never actually seen one have more than one address - but the data structure supports it). Is the resulting list of addresses supposed to contain loopback addresses? (I.e., does it on Windows? I don't have a Windows machine on which to test.) If so, then the patch that I submitted is deficient in that regard, as it will never return addresses from loopback interfaces. (Of course, the existing implementation and your patch will both return the magic loopback address rather than any addresses actually assigned to the interface, but that's a different problem.) In any case, either patch will solve the specific problem that I was trying to solve (getting the correct IP address displayed when hosting a Diablo 2 game), so in that respect I don't care which one makes it into a release. (In fact, now that I have a Wineskin with a custom wine build that works for me, I don't care if it ever makes it into a release - but I assume other people who don't have the ability to fix it themselves would like it, which is why I submitted a patch in the first place.)
About your patch I just would like to say that you should follow the surrounding code style, so the { goes in the next line. I tested it on Linux and it works for Heroes of Might & Magic 2 (https://bugs.winehq.org/show_bug.cgi?id=22819).
I was following the style used by 4 of the 7 opening braces in WS_gethostbyname (for which WS_get_local_ips is a helper, and which appears immediately following WS_get_local_ips in socket.c). Perhaps someone should change the 10% of the open braces in this file that do not appear alone on their lines if they are all meant to use a consistent style? And speaking of style, I suppose the indentation should be 4 spaces rather than whatever default my editor used? Jamie
Am Dienstag, 24. November 2015 schrieb Jamie Taylor:
Is the resulting list of addresses supposed to contain loopback addresses? (I.e., does it on Windows? I don't have a Windows machine on which to test.)
It seems that this is the case -- gethostbyname(gethostname()) returned 127.0.0.1 as the only address in a VMware VM with network adapters removed. Currently, Wine does not seem to replicate that behavior. I have been watching this for some time, as I use an application that crashes for this reason -- unless an Ethernet cable is plugged in, gethostbyname(gethostname()) returns NULL on Wine (which the native Linux version of gethostbyname also does, for that matter). I haven't dug into ws2_32 enough to be able to fix this, but if there is to be a Definitive Fix(TM) for this function, this behavior should probably be looked at. Joachim
On Wed, Nov 25, 2015 at 6:55 AM, Joachim Priesner <joachim.priesner(a)web.de> wrote:
Am Dienstag, 24. November 2015 schrieb Jamie Taylor:
Is the resulting list of addresses supposed to contain loopback addresses? (I.e., does it on Windows? I don't have a Windows machine on which to test.)
It seems that this is the case -- gethostbyname(gethostname()) returned 127.0.0.1 as the only address in a VMware VM with network adapters removed.
You mean a Windows VMWare, right? XP?
Currently, Wine does not seem to replicate that behavior. I have been watching this for some time, as I use an application that crashes for this reason -- unless an Ethernet cable is plugged in, gethostbyname(gethostname()) returns NULL on Wine (which the native Linux version of gethostbyname also does, for that matter). I haven't dug into ws2_32 enough to be able to fix this, but if there is to be a Definitive Fix(TM) for this function, this behavior should probably be looked at.
There is still things missing in our current knowledge, for example we have a Win 2008 test VM with 2 network adapters and it returns the default route as second IP instead of first in gethostbyname.
On Wed, Nov 25, 2015 at 3:57 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
There is still things missing in our current knowledge, for example we have a Win 2008 test VM with 2 network adapters and it returns the default route as second IP instead of first in gethostbyname.
Oops, both interfaces are default routes so that is OK.
On Wed, Nov 25, 2015 at 3:57 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
On Wed, Nov 25, 2015 at 6:55 AM, Joachim Priesner <joachim.priesner(a)web.de> wrote:
Am Dienstag, 24. November 2015 schrieb Jamie Taylor: It seems that this is the case -- gethostbyname(gethostname()) returned 127.0.0.1 as the only address in a VMware VM with network adapters removed.
You mean a Windows VMWare, right? XP?
I just confirmed this by disabling all network interfaces in XP, so it is something that we could implement too.
On Wed, Nov 25, 2015 at 04:45:28PM +0800, Bruno Jesus wrote:
On Wed, Nov 25, 2015 at 3:57 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
On Wed, Nov 25, 2015 at 6:55 AM, Joachim Priesner <joachim.priesner(a)web.de> wrote:
It seems that this is the case -- gethostbyname(gethostname()) returned 127.0.0.1 as the only address in a VMware VM with network adapters removed. I just confirmed this by disabling all network interfaces in XP, so it is something that we could implement too.
Interesting. Does GetAdaptersInfo return a non-null PIP_ADAPTER_INFO when there are no adapters on the (virtual) machine? How about GetIpAddrTable? Does the list of addresses returned by gethostbyname(gethostname()) include the loopback address when there _are_ other adapters configured with IPv4 addresses? (And if there are multiple loopback adapters installed/configured then are all of their addresses included?)
Am Mittwoch, 25. November 2015 schrieb Jamie Taylor:
Interesting. Does GetAdaptersInfo return a non-null PIP_ADAPTER_INFO when there are no adapters on the (virtual) machine? How about GetIpAddrTable?
GetAdaptersInfo fails with ERROR_NO_DATA in this case. GetIpAddrTable returns the loopback IP address.
Does the list of addresses returned by gethostbyname(gethostname()) include the loopback address when there _are_ other adapters configured with IPv4 addresses?
No. (However, if there is a loopback adapter added manually, its address is returned, just like with all other adapters.)
(And if there are multiple loopback adapters installed/configured then are all of their addresses included?)
Yes. Full test results in a Windows 8.1 VMware VM are here: http://pastebin.com/EjD5rec1 Joachim
On Thu, Nov 26, 2015 at 6:02 AM, Joachim Priesner <joachim.priesner(a)web.de> wrote:
Am Mittwoch, 25. November 2015 schrieb Jamie Taylor:
Interesting. Does GetAdaptersInfo return a non-null PIP_ADAPTER_INFO when there are no adapters on the (virtual) machine? How about GetIpAddrTable?
GetAdaptersInfo fails with ERROR_NO_DATA in this case. GetIpAddrTable returns the loopback IP address.
Does the list of addresses returned by gethostbyname(gethostname()) include the loopback address when there _are_ other adapters configured with IPv4 addresses?
No. (However, if there is a loopback adapter added manually, its address is returned, just like with all other adapters.)
(And if there are multiple loopback adapters installed/configured then are all of their addresses included?)
Yes.
Full test results in a Windows 8.1 VMware VM are here: http://pastebin.com/EjD5rec1
Thanks for the test, what is a manually added loopback adapter? Is there any use for it? I'm working on a patch that seems to replicate all current known tests and probably will get rid of the magic loopback address (127.12.34.56) unless someone can really explain why it exists.
Am Donnerstag, 26. November 2015 schrieb Bruno Jesus:
Thanks for the test, what is a manually added loopback adapter?
You can install a "Microsoft KM-Test Loopback Adapter" as network adapter. Although I guess it is treated like any other adapter by the OS (as the tests show). Joachim
On Wed, Nov 25, 2015 at 5:57 AM, Jamie Taylor <Jamie.Taylor(a)pobox.com> wrote:
On Tue, Nov 24, 2015 at 08:30:13PM +0800, Bruno Jesus wrote:
Hi, Jamie. Thanks for your effort, as you may have observed in the patches list your patch is marked as deferred because we are in code freeze for Wine 1.8 release. This code was touched a few times in attempts to fix this same issue you are addressing so maybe this time it will be the definitive fix =)
Hello, Bruno. Thanks for your review.
I think that a true definitive fix would include removing the magic loopback address completely, as it's obviously a hack (and not a value that would ever be returned by Windows). That seems like a much more invasive change.
I agree to that, the magic IP is used when the interface does not have an IP but has to be returned, that is why it was invented I believe.
I have a similar (not so robust) old patch that I used on FreeBSD (and also works in Linux), could you try it in OSX just out of curiosity, please? It has printf to help showing the results so a make test in the ws2_32/tests folder is enough to check if it works.
Yes, that patch works for me. The main difference is that it includes the magic loopback address in the returned list if there are any routes to a loopback interface in the forwarding table (actually, one copy of the magic loopback address per loopback interface, I think... but there's generally only one loopback interface). And it doesn't return all of the addresses from an interface (although I've never actually seen one have more than one address - but the data structure supports it).
I'll make some manual tests related about multiple IP addresses in the same interface and to check if the loopback address should really be returned.
Is the resulting list of addresses supposed to contain loopback addresses? (I.e., does it on Windows? I don't have a Windows machine on which to test.) If so, then the patch that I submitted is deficient in that regard, as it will never return addresses from loopback interfaces. (Of course, the existing implementation and your patch will both return the magic loopback address rather than any addresses actually assigned to the interface, but that's a different problem.)
In any case, either patch will solve the specific problem that I was trying to solve (getting the correct IP address displayed when hosting a Diablo 2 game), so in that respect I don't care which one makes it into a release. (In fact, now that I have a Wineskin with a custom wine build that works for me, I don't care if it ever makes it into a release - but I assume other people who don't have the ability to fix it themselves would like it, which is why I submitted a patch in the first place.)
I believe we need more tests to understand this problem better but your patch is better than mine because it simplifies the logic and removes the realloc so I would prefer something like it but we still need to test the loopback thing.
About your patch I just would like to say that you should follow the surrounding code style, so the { goes in the next line. I tested it on Linux and it works for Heroes of Might & Magic 2 (https://bugs.winehq.org/show_bug.cgi?id=22819).
I was following the style used by 4 of the 7 opening braces in WS_gethostbyname (for which WS_get_local_ips is a helper, and which appears immediately following WS_get_local_ips in socket.c). Perhaps someone should change the 10% of the open braces in this file that do not appear alone on their lines if they are all meant to use a consistent style?
Slowly we are changing into that direction but we don't do style only changes, so only when the code is really touched we will change that.
And speaking of style, I suppose the indentation should be 4 spaces rather than whatever default my editor used?
Yes, in this DLL we use four spaces I believe most of the time. There may be some tabs still around. Bruno
participants (3)
-
Bruno Jesus -
Jamie Taylor -
Joachim Priesner