Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed ntoa assumption on endianness #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geky
Copy link

@geky geky commented Feb 12, 2016

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
Copy link
Contributor

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: #44.

@geky
Copy link
Author

geky commented Feb 29, 2016

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
Copy link
Contributor

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
Copy link
Author

geky commented Feb 29, 2016

Agreed, that's what this pull request fixes.

Sorry if the title was poorly worded.

@bremoran
Copy link
Contributor

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
Copy link
Contributor

bogdanm commented Mar 1, 2016

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

@bremoran
Copy link
Contributor

bremoran commented Mar 1, 2016

@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
Copy link
Contributor

bremoran commented Mar 1, 2016

@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);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants