chriskohlhoff / asio

Asio C++ Library
http://think-async.com/Asio
4.96k stars 1.22k forks source link

ip::address::make_address() false positives (windows only) #274

Open mellery451 opened 6 years ago

mellery451 commented 6 years ago

the following addresses don't produce errors on windows:

    using boost::asio;
    boost::system::error_code ec;
    auto a1 = ip::address::make_address("1", ec); // WINDOWS: ec is false and a1 is 0.0.0.1
    auto a2 = ip::address::make_address("1.2", ec); // WINDOWS: ec is false and a2 is 1.0.0.2
    auto a3 = ip::address::make_address("1.2.3", ec); // WINDOWS: ec is false and a3 is 1.2.0.3

The same code will correctly fail on mac and linux. Is this a bug or a grey area in the definition of valid addresses?

davidchall commented 4 years ago

I’ve also run into this problem. Looks like the Windows library that converts IP address strings to integers has different behavior from other platforms.

More details here: https://superuser.com/a/486936

d12fk commented 2 years ago

I just ran into a similar and related issue, where ip::address::make_address("[1::1]:42", ec) produces different results on Windows and elsewhere.

I’ve also run into this problem. Looks like the Windows library that converts IP address strings to integers has different behavior from other platforms.

The cause for this is the WinSock2 API WSAStringToAddress function, which is used as the impl for inet_pton on Windows here:

https://github.com/chriskohlhoff/asio/blob/f70f65ae54351c209c3a24704624144bfe8e70a3/asio/include/asio/detail/impl/socket_ops.ipp#L2689-L2700

Since WSAStringToAddress returns a struct sockaddr* one could argue that it is the correct behavior to parse the port number as well, but then it's still different from the behavior on POSIX OSes, where inet_pton(3) is used.

Have not tried, but from the looks of it an easy fix could be to use the inet_pton WinSock2 function instead, since it returns a struct in(6)_addr as well.

d12fk commented 2 years ago
    using boost::asio;
    boost::system::error_code ec;
    auto a1 = ip::address::make_address("1", ec); // WINDOWS: ec is false and a1 is 0.0.0.1
    auto a2 = ip::address::make_address("1.2", ec); // WINDOWS: ec is false and a2 is 1.0.0.2
    auto a3 = ip::address::make_address("1.2.3", ec); // WINDOWS: ec is false and a3 is 1.2.0.3

WSAStringToAddress seems to use inet_aton(3) internally (or emulates it). There it is legal to abbreviate IPv4 addresses, with them getting zero stuffed like above. This is not supported in inet_pton(3), thus the difference in behavior. However, using inet_pton on Windows has the drawback that a scope_id in a v6 address needs to be handled manually, like it's done for non-Windows.

EDIT: a better alternative to inet_pton() might be to use getaddrinfo() (with AI_NUMERICHOST) (POSIX, MSDN). Downside here is that it allocates on the heap, but scope_id should be supported.

Note that #638 is the inverse of this issue, where the reporter asks for that behavior on MacOS.

d12fk commented 2 years ago

So, I guess it's up to @chriskohlhoff which behavior he wants in ASIO with regards to v4 addresses. I think it would be beneficial to have the same behavior on all OSes, but a fix will look differently depending on what address strings are to be supported.

vinipsmaker commented 1 year ago

Usually I'd advocate to just do whatever the underlying platform does and propagate the error back to the user. IO is not consistent among different platforms and Boost.Asio is all about IO. Boost.Asio portability is about offering the same interfaces to access the same features and not to ensure everyone will get the same implementation for the TCP stack.

However in this case I think behavior should be uniform among all platforms even if Boost.Asio has to implement its own parser. That's just parsing IP addresses. A parser has predictable behavior and so should the interface. This inconsistency will just inadvertently introduce bugs in the code of cross-platforms projects of uninformed programmers for no good reason.

On the other hand we still have to decide which behavior is correct. This will be a big debate by itself and good arguments are sure to follow on any side of the balance. Maybe what Boost.Asio should do is to adopt the behavior that could be backwards in the future so it's free to make changes if it takes the wrong road.

For instance, if it adopts the strict approach today and start to only accept legally valid IPv4 addresses, it could still change its mind tomorrow to be more liberal without breaking previous code.