erts: support endian-aware packet framing - #11441
Conversation
CT Test ResultsNo tests were run for this PR. This is either because the build failed, or the PR is based on a branch without GH actions tests configured. Results for commit 7dea5d4 To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
38d5264 to
122cfae
Compare
122cfae to
3d73e92
Compare
3d73e92 to
b1c8539
Compare
| if (tp[2] == am_little) { | ||
| prefix->byte_order = ERTS_PACKET_BYTE_ORDER_LITTLE; | ||
| } else if (tp[2] != am_big) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Thanks for the updates to cover all the different variants, I could've sworn the previous implementation had native endian as well, but I can't find it here anymore. Can you add that as well everywhere? It's fairly easy to implement along the lines of:
| if (tp[2] == am_little) { | |
| prefix->byte_order = ERTS_PACKET_BYTE_ORDER_LITTLE; | |
| } else if (tp[2] != am_big) { | |
| return 0; | |
| } | |
| if (tp[2] == am_little) { | |
| prefix->byte_order = ERTS_PACKET_BYTE_ORDER_LITTLE; | |
| } else if (tp[2] == am_native) { | |
| #if !defined(WORDS_BIGENDIAN) | |
| prefix->byte_order = ERTS_PACKET_BYTE_ORDER_LITTLE; | |
| #endif | |
| } else if (tp[2] != am_big) { | |
| return 0; | |
| } |
There was a problem hiding this comment.
Of course! And I'm simplifying my code changes! when ready, I will change the PR status
b1c8539 to
6a9c7c4
Compare
| switch (packet[2]) { | ||
| case am_big: | ||
| switch (packet_bytes) { | ||
| case 2: type = TCP_PB_2_BIG; break; | ||
| case 3: type = TCP_PB_3_BIG; break; | ||
| case 4: type = TCP_PB_4_BIG; break; | ||
| } | ||
| break; | ||
| case am_little: | ||
| switch (packet_bytes) { | ||
| case 2: type = TCP_PB_2_LITTLE; break; | ||
| case 3: type = TCP_PB_3_LITTLE; break; | ||
| case 4: type = TCP_PB_4_LITTLE; break; | ||
| } | ||
| break; | ||
| case am_native: | ||
| switch (packet_bytes) { | ||
| case 2: type = TCP_PB_2_NATIVE; break; | ||
| case 3: type = TCP_PB_3_NATIVE; break; | ||
| case 4: type = TCP_PB_4_NATIVE; break; | ||
| } | ||
| break; |
There was a problem hiding this comment.
The implementation can be simplified a bit by removing TCP_PB_2_NATIVE et al and changing the switch as follows:
| switch (packet[2]) { | |
| case am_big: | |
| switch (packet_bytes) { | |
| case 2: type = TCP_PB_2_BIG; break; | |
| case 3: type = TCP_PB_3_BIG; break; | |
| case 4: type = TCP_PB_4_BIG; break; | |
| } | |
| break; | |
| case am_little: | |
| switch (packet_bytes) { | |
| case 2: type = TCP_PB_2_LITTLE; break; | |
| case 3: type = TCP_PB_3_LITTLE; break; | |
| case 4: type = TCP_PB_4_LITTLE; break; | |
| } | |
| break; | |
| case am_native: | |
| switch (packet_bytes) { | |
| case 2: type = TCP_PB_2_NATIVE; break; | |
| case 3: type = TCP_PB_3_NATIVE; break; | |
| case 4: type = TCP_PB_4_NATIVE; break; | |
| } | |
| break; | |
| switch (packet[2]) { | |
| #if defined(WORDS_BIGENDIAN) | |
| case am_native: | |
| #endif | |
| case am_big: | |
| switch (packet_bytes) { | |
| case 2: type = TCP_PB_2_BIG; break; | |
| case 3: type = TCP_PB_3_BIG; break; | |
| case 4: type = TCP_PB_4_BIG; break; | |
| } | |
| break; | |
| #if !defined(WORDS_BIGENDIAN) | |
| case am_native: | |
| #endif | |
| case am_little: | |
| switch (packet_bytes) { | |
| case 2: type = TCP_PB_2_LITTLE; break; | |
| case 3: type = TCP_PB_3_LITTLE; break; | |
| case 4: type = TCP_PB_4_LITTLE; break; | |
| } | |
| break; |
|
|
||
| #endif | ||
| ERTS_GLB_INLINE void | ||
| erts_sys_driver_reverse_packet_header(byte *header, int packet_bytes); |
There was a problem hiding this comment.
Changing endian with this function before/after the get/put operations makes the code rather hard to follow, I think it's better to be explicit with put_xx_little / get_xx_little / put_xx / get_xx instead.
There was a problem hiding this comment.
I think I'm ready now. Thank you for your review.
6a9c7c4 to
e1d9c9c
Compare
Parse packet byte order in open_port and use explicit endian-specific get and put operations in both sys-driver backends. Verify both wire directions and cached Windows fd reuse with behavioral port tests.
e1d9c9c to
65bc1b9
Compare
Accept endian-qualified packet widths in decode_packet and both TCP backends. Resolve native byte order at the option boundary and retain only concrete big- and little-endian parser types.
Apply the packet contract to SSL connection options, runtime setopts, and framing. A synchronized raw TLS peer verifies exact wire bytes without allowing matching encoder and decoder mistakes to cancel out.
Document three-byte headers and explicit big, little, and native tuple forms for open_port and decode_packet. Keep scalar packet widths as the backward-compatible big-endian shorthand.
Declare the term-spec bounds only when SCTP support is built, since their consumers are compiled out otherwise.
65bc1b9 to
7dea5d4
Compare
| ASSERT(desc->inet.htype == TCP_PB_HTTP_BIN || | ||
| desc->inet.htype == TCP_PB_HTTPH_BIN); | ||
| if (desc->inet.htype == TCP_PB_HTTP_BIN || | ||
| desc->inet.htype == TCP_PB_HTTPH_BIN) { |
There was a problem hiding this comment.
This is a behavioral change, the assertion was obviously wrong (without ever firing) but we cannot blindly correct the code to fit it. The PS team needs to take a look. @IngelaAndin
There was a problem hiding this comment.
Yes, I tried to fix it because it didn't fire. Let me revert
| int htype_changed; | ||
|
|
||
| htype_changed = | ||
| (desc->stype == SOCK_STREAM) && (desc->htype != old_htype); | ||
| if (htype_changed) { | ||
| tcp_descriptor *tdesc = (tcp_descriptor *) desc; | ||
| /* Reparse buffered input using the new header type. */ | ||
| tdesc->i_remain = 0; | ||
| } |
There was a problem hiding this comment.
What's the purpose of this change (and the related ones further down)?
There was a problem hiding this comment.
It was for lifecycle bug fix: inet:setopts(Socket, [{packet, NewMode}]) wasn't able to change the recv's mode. But it's out-of-scope of this PR. Sorry. It came out while fixing tests. I should've been more careful.
There was a problem hiding this comment.
Nobody's run into that bug so far, so I'd say there's no problem keeping it in this PR, just break it out into a separate commit and write a message for it (and maybe an improved testcase just for targeting this). We can always backport it if there's a need.
| if (dd->packet_header_endianness == | ||
| ERTS_SYS_DRIVER_PACKET_HEADER_ENDIAN_LITTLE) | ||
| put_little_int16(len, lbp); | ||
| else | ||
| put_int16(len, lbp); |
There was a problem hiding this comment.
Nitpick (applies elsewhere too):
| if (dd->packet_header_endianness == | |
| ERTS_SYS_DRIVER_PACKET_HEADER_ENDIAN_LITTLE) | |
| put_little_int16(len, lbp); | |
| else | |
| put_int16(len, lbp); | |
| if (dd->packet_header_endianness == | |
| ERTS_SYS_DRIVER_PACKET_HEADER_ENDIAN_LITTLE) { | |
| put_little_int16(len, lbp); | |
| } else { | |
| put_int16(len, lbp); | |
| } |
| ASSERT(0); | ||
| return; |
There was a problem hiding this comment.
| ASSERT(0); | |
| return; | |
| ERTS_UNREACHABLE; |
| ASSERT(0); | ||
| driver_failure_posix(dp->port_num, EINVAL); | ||
| driver_free_binary(bin); | ||
| return; |
There was a problem hiding this comment.
| ASSERT(0); | |
| driver_failure_posix(dp->port_num, EINVAL); | |
| driver_free_binary(bin); | |
| return; | |
| ERTS_UNREACHABLE; |
| int aid; | ||
| int req; | ||
| int i = 0; | ||
| #ifdef HAVE_SCTP |
There was a problem hiding this comment.
This one came from ./otp_build check warning, unused variable, discovered while passing CI in local. should I revert?
There was a problem hiding this comment.
This is fine, just break it into a separate commit.
open_port/2,gen_tcp, anderlang:decode_packet/3only support big-endian length prefixes, forcing little-endian protocols to implement framing themselves.Add three-byte packet headers and explicit big- or little-endian framing for two-, three-, and four-byte prefixes. Port and TCP options use
{packet, {N, Endian}}, whiledecode_packet/3accepts{N, Endian}. Existing scalar packet forms remain big-endian. The implementation covers spawned and file-descriptor ports, bothgen_tcpbackends, documentation, and regression tests.Closes #11411