ponyc
ponyc copied to clipboard
ponylang
UDPSocket incorrectly maps sockname to localhost
UDPSocket.create stores a copy of the local sockaddr, but then calls map_any_to_loopback in pony_os_sockname, which converts INADDR_ANY to INADDR_LOOPBACK (and the equivalent for IPv6). This results in an incorrect value from UDPSocket.local_address, and would probably affect UDPSocket.set_broadcast which also uses the field.
https://gist.github.com/anacrolix/dea5407822c317abb09c40416c0d2368
By modifying host, on line 11, you can see it occurs for "" and "::" too. If you check with lsof, or similar, you'll see that the socket is bound correctly however, and not to localhost.
The mentioned code where this is happening is: https://github.com/ponylang/ponyc/blob/1da8604fb6d779083816303c35794d92fbb075b0/src/libponyrt/lang/socket.c#L122-L126
We discussed this briefly on the sync call.
It's not clear to me off the top of my head why the INADDR_ANY is used in this case for UDP or what the expected behavior is.
This would need more research (which we didn't have time for on the sync call).
So, is the issue here that you are expecting a specific value for local address? Binding to 0.0.0.0 can bind to multiple addresses and you don't know what you will get for local_address in that case.
Is the problem that local_address only returns 1 address rather than all possible ones that are bound to when you do "0.0.0.0"?
You say "incorrect value". Can you expand on that? What you are getting for a value and what you did you expect to get?
@SeanTAllen @jemc It's standard practice to use INADDR_ANY like this. 0.0.0.0 is the textual representation for INADDR_ANY. The local address after binding to INADDR_ANY, is INADDR_ANY, which is 0.0.0.0 for IPv4, and :: for IPv6. Removing the mapping line fixes this, and local_address returns the correct value.
During sync, we took a further look at this. We need to ask Sylvan about the reasoning for this if he remembers.
We don't feel particularly confident in the test coverage around the usage map_any_to_loopback to know how much should be changed or if we would notice breakage.
More investigation basically.
I found these related commits, but we don't understand the reason they were done:
Sylvan added this to os_connect as inline logic:
- https://github.com/ponylang/ponyc/commit/465466b0bea366159eeb2f2dd88a7016ce4e9472
Andy McNeil made it a function to use it in Windows UDP:
- https://github.com/ponylang/ponyc/commit/98d10976c627c0881fd387045b1f61dd581be899
Sylvan added it to other address resolution cases:
- https://github.com/ponylang/ponyc/commit/cce6183284bd7bbc47b9e56c17987941713e9070
@SeanTAllen and I discussed this a bit today.
It still needs further thought (perhaps in the next sync call) but my basic position is:
- we should remove all the current use sites of
map_any_to_loopback- we should not magically convert "any" to "loopback" implicitly - for convenience (e.g. tests that hold two TCP or UDP sockets and want to easily use one to talk to the other using the local/listen address of one), we introduce a new explicit method that you can call on
NetAddresswhich will explicitly map "any" to "loopback" because you asked for that to happen.
This will be a breaking change for code that relies on this (probably tests for networking code are the most common use case), but adding the convenience method with some release notes explaining it will make it easy to fix the breakages without a lot of effort once those breakage sites are identified.
Note that Joe's position is also mine modulo our working out specific details. If anyone wants to weigh in with ideas for details or otherwise, do let us know. I hope to have a plan in place for soon so someone could do the work.
im marking this ready for work. really, there might be some issues that arise but i think we have a path forward here where work can be started and additional consultation done.