search

ponylang / ponyc

Pony is an open-source, actor-model, capabilities-secure, high performance programming language
http://www.ponylang.io
BSD 2-Clause "Simplified" License
5.74k stars 415 forks source link

UDPSocket incorrectly maps sockname to localhost #3580

Open anacrolix opened 4 years ago

anacrolix commented 4 years ago

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.

SeanTAllen commented 4 years ago

Can you include a minimal example of your UDP code?

anacrolix commented 4 years ago

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.

jemc commented 4 years ago

The mentioned code where this is happening is: https://github.com/ponylang/ponyc/blob/1da8604fb6d779083816303c35794d92fbb075b0/src/libponyrt/lang/socket.c#L122-L126

jemc commented 4 years ago

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).

SeanTAllen commented 4 years ago

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?

anacrolix commented 4 years ago

@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.

SeanTAllen commented 4 years ago

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.

jemc commented 4 years ago

I found these related commits, but we don't understand the reason they were done:

Sylvan added this to os_connect as inline logic:

Andy McNeil made it a function to use it in Windows UDP:

Sylvan added it to other address resolution cases:

jemc commented 2 years ago

@SeanTAllen and I discussed this a bit today.

It still needs further thought (perhaps in the next sync call) but my basic position is:

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.

SeanTAllen commented 2 years ago

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.

SeanTAllen commented 2 years ago

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.