From 00793dd21bd6e79f98706bd28ee9ff7ba52b3260 Mon Sep 17 00:00:00 2001 From: Emil Dahl Juhl Date: Mon, 14 Jul 2025 12:28:43 +0200 Subject: [PATCH] Protect transportWrite with writeMutex Guard usage of the `transportWrite` and encoding frames into the `writeBuffer` by adding a `writeFrameMutex` to be locked within `writeFrame`. The `write` function relies on repeated calls to `read` that need to be performed by a separate thread from the user of hdlcpp. This is because `write` will block until a `frameAck` or `frameNack` is received or a timeout is hit. The hdlcpp implementation, however, also performs writes on the transport layer from within the `read` function itself. It does so in the process of either sending a `frameAck` or `frameNack` upon receival of a data frame. From this it is aparent that any user of hdlcpp is basically forced into a race condition that can potentially mangle frames on the wire. The race can be hit by simply receiving data frames while trying to transmit data frames as well. Since the function which actually encodes data into the `writeBuffer` and calls the `transportWrite` callback is `writeFrame`, adding a mutex to specifically guard this function can prevent the race. This makes sure that if the thread handling `read` were to attempt transmitting a `frameAck` while another thread was attempting to transmit a data frame, then these two frames don't get mixed together. By introducing a new mutex rather than just moving the existing `writeMutex`, the thread safety of the sequence numbering increments within `write` is kept as-is. --- include/Hdlcpp.hpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/Hdlcpp.hpp b/include/Hdlcpp.hpp index fc830dc..72389f6 100644 --- a/include/Hdlcpp.hpp +++ b/include/Hdlcpp.hpp @@ -440,6 +440,8 @@ class Hdlcpp { int writeFrame(TransportAddress address, Frame frame, uint8_t sequenceNumber, ConstContainer data) { + std::lock_guard writeLock(writeFrameMutex); + int result; if ((result = encode(address, frame, sequenceNumber, data, { writeBuffer })) < 0) @@ -550,7 +552,11 @@ class Hdlcpp { static constexpr uint8_t FlagSequence = 0x7e; static constexpr uint8_t ControlEscape = 0x7d; + // Whenever writeMutex and writeFrameMutex both needs to be locked from the + // same thread, writeMutex must be locked first. std::mutex writeMutex; + std::mutex writeFrameMutex; + TransportRead transportRead; TransportWrite transportWrite; Buffer readBuffer;