ARMmbed / sal

mbed C abstract socket API layer
Other
3 stars 16 forks source link

Removed ntoa assumption on endianness #42

Open geky opened 8 years ago

geky commented 8 years ago

This is a fix for what was a rather entertaining bug.

inet_ntoa(inet_aton("1.2.3.4")) -> "4.3.2.1"

Even if it had worked, network code probably shouldn't be relying processor endianness.

bremoran commented 8 years ago

You're right that there's an assumption in this code: that all addresses are stored in big-endian order. This is not a processor assumption, it's an assumption made so that the API can be common across underlying stacks.

Unfortunately, it's not documented; I've just raised an issue on this: https://github.com/ARMmbed/sal/issues/44.

geky commented 8 years ago

That assumption is broken: https://github.com/ARMmbed/sal/blob/master/source/inet_ntoa.c#L37

The following code changes behaviour based on your processor (in_addr_t is aliased to uint32_t):

in_addr_t ia = socket_addr_get_ipv4_addr(&ina);
unsigned char *ucp = (unsigned char *)&ia;

At the very least, inet_aton and inet_ntoa should not be asymmetric.

bremoran commented 8 years ago

In that case, inet_ntoa is broken. Big-endian order addresses is a requirement (albeit not a documented one) of the socket abstraction layer.

geky commented 8 years ago

Agreed, that's what this pull request fixes.

Sorry if the title was poorly worded.

bremoran commented 8 years ago

I think I understand what's going on now. inet_ntoa and inet_aton were not updated to work with the assumptions of the socket abstraction layer. On the other hand, inet_pton and inet_ntop were updated for these assumptions and work natively with struct socket_addr.

I'm inclined to mark inet_ntoa and inet_aton as deprecated.

@bogdanm what do you think?

bogdanm commented 8 years ago

Is it possible/easy to implement inet_ntoa in terms of inet_ntop (to provide a backwards compatible option) ?

bremoran commented 8 years ago

@bogdanm That might be viable, yes. They seem to have the same semantics when AF_INET is provided to inet_pton, minus some return code irregularities that should be easy to fix.

bremoran commented 8 years ago

@geky: could you try again with this replacement for inet_aton?

int
inet_aton(const char *cp, struct socket_addr *addr)
{
    int rc = inet_pton(AF_INET, cp, addr);
    return (rc == 1 ? 1 : 0);
}