Fix byte ordering in sockets API for systems where host byte order == network byte order #1327
Conversation
loganek
commented
Jul 25, 2022
loganek
commented
- Documented data structures used for storing IP addresses.
- Fixed bug in bind and connect methods by updating a code in wasi_socket_ext.
loganek
force-pushed
the
loganek/fix-byte-order
branch
from
a9907cb to
fb0d9b7
Compare
… network byte order - Documented data structures used for storing IP addresses. - Fixed bug in bind and connect methods by updating a code in wasi_socket_ext.
loganek
force-pushed
the
loganek/fix-byte-order
branch
from
fb0d9b7 to
3b93fa3
Compare
| out->addr.ip4.addr.n1 = (addr_num & 0x00FF0000) >> 16; | ||
| out->addr.ip4.addr.n2 = (addr_num & 0x0000FF00) >> 8; | ||
| out->addr.ip4.addr.n3 = (addr_num & 0x000000FF); | ||
| } |
There was a problem hiding this comment.
let's take a walk:
addr_numas a parameter, is the big-endian- L25,
addr_numis host-byte-order - L28 - L31, store the MSB to
n0and the LSB ton3. So back to big-endian?
There was a problem hiding this comment.
Yes, n0 in this case is the most significant byte (e.g. given 127.0.0.1, n0 is 127), which is the same behavior as we have before this PR. What this PR fixes though is the case when the host system is big endian. Even though big endian system is currently not that popular, I think we should write the code in a way it supports both architectures.
There was a problem hiding this comment.
- If the host system is little-endian, what we are doing is big-endian -> little-endian -> big-endian ?
- if the host system is big-endian, what we are doing is big-endian -> big-endian -> big-endian?
So, why not directly store the parameter addr_num into _wasi_addr_t in network-byte-order?
There was a problem hiding this comment.
I'm no sure if we can serialise addrnum to wasi_addr_t directly - as this is violation of strict aliasing rules. So I think more accurate description of what we do here is:
Little-endian host: big endian -> little endian (through ntohl) -> wasi_addr_t
Big-endian host: big endian -> no-op (as ntohl in that case shoud do nothing) -> wasi_addr_t
So there's no redundant operation in any of the stages.
There was a problem hiding this comment.
If we want to stick to using & operator for calculating each byte, w have to make sure the value is in the host byte order. The alternative here would be to convert addr_num to array of uint8 so we can safely rely on network byte order, but I find this approach a bit more elegant (that's very subjective though)
Implement more socket APIs, refer to #1336 and below PRs: - Implement wasi_addr_resolve function (#1319) - Fix socket-api byte order issue when host/network order are the same (#1327) - Enhance sock_addr_local syscall (#1320) - Implement sock_addr_remote syscall (#1360) - Add support for IPv6 in WAMR (#1411) - Implement ns lookup allowlist (#1420) - Implement sock_send_to and sock_recv_from system calls (#1457) - Added http downloader and multicast socket options (#1467) - Fix `bind()` calls to receive the correct size of `sockaddr` structure (#1490) - Assert on correct parameters (#1505) - Copy only received bytes from socket recv buffer into the app buffer (#1497) Co-authored-by: Marcin Kolny <[email protected]> Co-authored-by: Marcin Kolny <[email protected]> Co-authored-by: Callum Macmillan <[email protected]>
…ytecodealliance#1327) Fix socket-api byte order issue for systems where host byte order and network byte order are the same: - Document data structures used for storing IP addresses - Fix bug in bind and connect methods by updating code in wasi_socket_ext
Implement more socket APIs, refer to bytecodealliance#1336 and below PRs: - Implement wasi_addr_resolve function (bytecodealliance#1319) - Fix socket-api byte order issue when host/network order are the same (bytecodealliance#1327) - Enhance sock_addr_local syscall (bytecodealliance#1320) - Implement sock_addr_remote syscall (bytecodealliance#1360) - Add support for IPv6 in WAMR (bytecodealliance#1411) - Implement ns lookup allowlist (bytecodealliance#1420) - Implement sock_send_to and sock_recv_from system calls (bytecodealliance#1457) - Added http downloader and multicast socket options (bytecodealliance#1467) - Fix `bind()` calls to receive the correct size of `sockaddr` structure (bytecodealliance#1490) - Assert on correct parameters (bytecodealliance#1505) - Copy only received bytes from socket recv buffer into the app buffer (bytecodealliance#1497) Co-authored-by: Marcin Kolny <[email protected]> Co-authored-by: Marcin Kolny <[email protected]> Co-authored-by: Callum Macmillan <[email protected]>
3 participants
