Worker: Ensure 4-byte alignment for network packet receive buffers and test buffers to avoid UB (undefined behavior)#1756
Open
Worker: Ensure 4-byte alignment for network packet receive buffers and test buffers to avoid UB (undefined behavior)#1756
Conversation
…d test buffers to avoid undefined behavior # Details - Fixes #1755 - All info in that ticket.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Member
Author
|
I've tested this branch in macOS and Linux (test.mediasoup.org) and it works fine. Nothing is broken and we are now ready to run mediasoup in exotic architectures that require strict alignment when accessing unit16 or unit32 fields from memory referenced by pointers. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
-O3in ASAN checks and fuzzer to not hide UB problems.More info
What is the real design problem we have? We obtain RTP, ICE, SCTP, etc. packets from the network and store those bytes in a buffer (this happens in
UdpSocketHandler.cpp, etc).Later we do a cast of that buffer to
RTP::Packetor other classes/structs and here the UB problems may happen.First, those classes/structs have members of different size (
uint8_t,uint16_t, sometimes fields ofuint32_t, etc). The whole struct will have an alignment that corresponds to the bigger field, so if the class/struct has auint32_tthe whole struct has alignment 4 (4 bytes).Second, when we create the read buffer (the static one) such a buffer maybe assigned a memory position that is NOT aligned to 4 bytes. In that case, when we later cast the buffer to a
RTP::Packet, when we dopacket->GetXxxx()we are not accessing through a 4 bytes aligned struct anymore. In som archs this produces SIGBUS or undefined behavior (we may read unexpected bytes instead of the intended ones).The solution in this PR consists on forcing all read buffers to be in positions of the memory that are aligned to 4 bytes because we know that all our RTP, STUN, RTCP, SCTP, etc. classes/structs are aligned to maximum 4 bytes (there are no
uint64_tfields in any of them.Other than that we are safe because all RTP, RTCP, STUN and SCTP structs are defined to be aligned in their RFCs (on purpose) so they all have alignment of 4 bytes (largest field in any of those structs is 32 bits so 4 bytes so 4).
So there are two different things here:
castto RTP, RTCP, STUN, SCTP structs/classes) are in positions of memory multiple of 4. We get this by declaring all those buffers withalignas(4).uint8_t foofield followed ofuint16_t bar. In this case the struct is aligned to 2 bytes (the largest field in the struct) so some archs may add padding of 1 byte afterfooto make the total size of the struct be 4 (1 byte offoo+ 1 byte of padding + 2 bytes ofbar). Then if youcasta buffer with data received from the network into a local variablemyStructVar(no matter it's declared withalignas(4)because that's irrelevant here) then you may have problems in some archs if you domyStructVar.barbecause you may end accessing the 3rd and 4th bytes of the buffer instead of the 2nd and 3rd as expected, so undefined behavior (UB) or even a SIGBUS in some archs.As said above we are fine because all the structs we use are aligned to 4 bytes on purpose in their RFCs.
However, the proper way to go would be that we completely avoid casting buffers with received bytes to our classes or structs. That's bad by design. AFAIS modern implementations do a
memcpyof the buffer into anstructonce they know which kind of packet is. Pseudo-code:RTP::PacketHeader rtpHeader{}; memcpy(&rtpHeader, readBuffer, readLen);This way, it's 100% guaranteed that the content in the read buffer is properly copied to
rtpHeaderbecause it includes all needed padding required by the struct design (its fields). However, I'm not sure this is valid in case we need to send such a packet in the wire since of course we MUST NOT send padding bytes within a RTP packet...