Fix 189, 196, 206 (#211)

Close #189
Close #196
Close #206

Author: pavel-kirienko

.github/workflows/main.yml

@@ -132,7 +132,7 @@ jobs:
       contains(github.event.head_commit.message, '#sonar')
     runs-on: ubuntu-latest
     env:
-      SONAR_SCANNER_VERSION: 4.6.1.2450
+      SONAR_SCANNER_VERSION: 4.8.0.2856
       SONAR_SERVER_URL: "https://sonarcloud.io"
       BUILD_WRAPPER_OUT_DIR: build_wrapper_output_directory
       GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
@@ -154,7 +154,7 @@ jobs:
         distribution: zulu
 
     - name: Cache SonarCloud packages
-      uses: actions/cache@v1
+      uses: actions/cache@v3
       with:
         path: ~/.sonar/cache
         key: ${{ runner.os }}-sonar
@@ -193,7 +193,7 @@ jobs:
         --define sonar.projectName=libcanard
         --define sonar.projectKey=libcanard
         --define sonar.sources=libcanard
-        --define sonar.exclusions=libcanard/cavl.h
+        --define sonar.exclusions=libcanard/_canard_cavl.h
         --define sonar.cfamily.gcov.reportsPath=.
         --define sonar.cfamily.build-wrapper-output="${{ env.BUILD_WRAPPER_OUT_DIR }}"
         --define sonar.host.url="${{ env.SONAR_SERVER_URL }}"

CONTRIBUTING.md

@@ -58,6 +58,9 @@ The following tools are required to conduct library development locally
 - CMake
 - An AMD64 machine
 
+You may want to use the [toolshed](https://github.com/OpenCyphal/docker_toolchains/pkgs/container/toolshed)
+container for this.
+
 ### Clang-Tidy
 
 Clang-Tidy is used to enforce compliance with MISRA and Zubax Coding Conventions.

README.md

@@ -244,6 +244,13 @@ If you find the examples to be unclear or incorrect, please, open a ticket.
 
 - Remove UB as described in [203](https://github.com/OpenCyphal/libcanard/issues/203).
 
+#### v3.0.2
+
+- Robustify the multi-frame transfer reassembler state machine
+  ([#189](https://github.com/OpenCyphal/libcanard/issues/189)).
+- Eliminate the risk of a header file name collision by renaming the vendored Cavl header to `_canard_cavl.h`
+  ([#196](https://github.com/OpenCyphal/libcanard/issues/196)).
+
 ### v2.0
 
 - Dedicated transmission queues per redundant CAN interface with depth limits.

libcanard/cavl.h libcanard/_canard_cavl.h

@@ -3,7 +3,7 @@
 /// Author: Pavel Kirienko <pavel@opencyphal.org>
 
 #include "canard.h"
-#include "cavl.h"
+#include "_canard_cavl.h"
 #include <string.h>
 
 // --------------------------------------------- BUILD CONFIGURATION ---------------------------------------------
@@ -849,10 +849,20 @@ CANARD_PRIVATE int8_t rxSessionUpdate(CanardInstance* const          ins,
     }
     else
     {
+        // The purpose of the correct_start check is to reduce the possibility of accepting a malformed multi-frame
+        // transfer in the event of a CRC collision. The scenario where this failure mode would manifest is as follows:
+        // 1. A valid transfer (whether single- or multi-frame) is accepted with TID=X.
+        // 2. All frames of the subsequent multi-frame transfer with TID=X+1 are lost except for the last one.
+        // 3. The CRC of said multi-frame transfer happens to yield the correct residue when applied to the fragment
+        //    of the payload contained in the last frame of the transfer (a CRC collision is in effect).
+        // 4. The last frame of the multi-frame transfer is erroneously accepted even though it is malformed.
+        // The correct_start check eliminates this failure mode by ensuring that the first frame is observed.
+        // See https://github.com/OpenCyphal/libcanard/issues/189.
         const bool correct_transport = (rxs->redundant_transport_index == redundant_transport_index);
         const bool correct_toggle    = (frame->toggle == rxs->toggle);
         const bool correct_tid       = (frame->transfer_id == rxs->transfer_id);
-        if (correct_transport && correct_toggle && correct_tid)
+        const bool correct_start     = frame->start_of_transfer || (rxs->total_payload_size > 0);
+        if (correct_transport && correct_toggle && correct_tid && correct_start)
         {
             out = rxSessionAcceptFrame(ins, rxs, frame, extent, out_transfer);
         }

libcanard/canard.c

@@ -2,7 +2,7 @@
 // Copyright (c) 2016-2020 OpenCyphal Development Team.
 // These tests have been adapted from the Cavl test suite that you can find at https://github.com/pavel-kirienko/cavl
 
-#include <cavl.h>
+#include <_canard_cavl.h>
 #include "catch.hpp"
 #include <algorithm>
 #include <array>

tests/test_private_cavl.cpp

@@ -331,3 +331,110 @@ TEST_CASE("RxSubscriptionErrors")
     REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardRxAccept(&ins.getInstance(), 0, &frame, 0, nullptr, nullptr));
     REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardRxAccept(nullptr, 0, nullptr, 0, nullptr, nullptr));
 }
+
+TEST_CASE("Issue189")  // https://github.com/OpenCyphal/libcanard/issues/189
+{
+    using helpers::Instance;
+    using exposed::RxSession;
+
+    Instance              ins;
+    CanardRxTransfer      transfer{};
+    CanardRxSubscription* subscription              = nullptr;
+    const std::uint8_t    redundant_transport_index = 0;
+
+    const auto accept = [&](const CanardMicrosecond          timestamp_usec,
+                            const std::uint32_t              extended_can_id,
+                            const std::vector<std::uint8_t>& payload) {
+        static std::vector<std::uint8_t> payload_storage;
+        payload_storage = payload;
+        CanardFrame frame{};
+        frame.extended_can_id = extended_can_id;
+        frame.payload_size    = std::size(payload);
+        frame.payload         = payload_storage.data();
+        return ins.rxAccept(timestamp_usec, frame, redundant_transport_index, transfer, &subscription);
+    };
+
+    ins.getAllocator().setAllocationCeiling(sizeof(RxSession) + 50);  // A session and the payload buffer.
+
+    // Create a message subscription.
+    CanardRxSubscription sub_msg{};
+    REQUIRE(1 == ins.rxSubscribe(CanardTransferKindMessage, 0b0110011001100, 50, 1'000'000, sub_msg));
+    REQUIRE(ins.getMessageSubs().at(0) == &sub_msg);
+    REQUIRE(ins.getMessageSubs().at(0)->port_id == 0b0110011001100);
+    REQUIRE(ins.getMessageSubs().at(0)->extent == 50);
+    REQUIRE(ins.getMessageSubs().at(0)->transfer_id_timeout_usec == 1'000'000);
+    REQUIRE(ensureAllNullptr(ins.getMessageSubs().at(0)->sessions));
+    REQUIRE(ins.getResponseSubs().empty());
+    REQUIRE(ins.getRequestSubs().empty());
+
+    // First, ensure that the reassembler is initialized, by feeding it a valid transfer at least once.
+    subscription = nullptr;
+    REQUIRE(1 == accept(100'000'001, 0b001'00'0'11'0110011001100'0'0100111, {0x42, 0b111'00000}));
+    REQUIRE(subscription != nullptr);
+    REQUIRE(subscription->port_id == 0b0110011001100);
+    REQUIRE(transfer.timestamp_usec == 100'000'001);
+    REQUIRE(transfer.metadata.priority == CanardPriorityImmediate);
+    REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage);
+    REQUIRE(transfer.metadata.port_id == 0b0110011001100);
+    REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
+    REQUIRE(transfer.metadata.transfer_id == 0);
+    REQUIRE(transfer.payload_size == 1);
+    REQUIRE(0 == std::memcmp(transfer.payload, "\x42", 1));
+    REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2);  // The SESSION and the PAYLOAD BUFFER.
+    REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
+    REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
+    ins.getAllocator().deallocate(transfer.payload);
+    REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);  // The payload buffer is gone.
+    REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));
+
+    // Next, feed the last frame of another transfer whose TID/TOG match the expected state of the reassembler,
+    // and the CRC matches the payload available in the last frame.
+    // This frame should be rejected because we didn't observe the first frame of this transfer.
+    // This failure mode may occur when the first frame is lost.
+    //
+    // Here's how we compute the reference value of the transfer CRC:
+    //  >>> from pycyphal.transport.commons.crc import CRC16CCITT
+    //  >>> CRC16CCITT.new(b'DUCK').value_as_bytes
+    //  b'4\xa3'
+    subscription = nullptr;
+    REQUIRE(0 == accept(100'001'001,  // The result should be zero because the transfer is rejected.
+                        0b001'00'0'11'0110011001100'0'0100111,           //
+                        {'D', 'U', 'C', 'K', '4', 0xA3, 0b011'00001}));  // SOF=0, EOF=1, TOG=1, TID=1, CRC=0x4A34
+    REQUIRE(subscription != nullptr);                                    // Subscription exists.
+    REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);         // The SESSION only.
+    REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));
+    REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
+
+    // Now feed a similar transfer that is not malformed. It should be accepted.
+    // Here's how we compute the reference value of the transfer CRC:
+    //  >>> from pycyphal.transport.commons.crc import CRC16CCITT
+    //  >>> CRC16CCITT.new(b'\x01\x02\x03\x04\x05\x06\x07DUCK').value_as_bytes
+    //  b'\xd3\x14'
+    subscription = nullptr;
+    REQUIRE(0 == accept(100'002'001,  //
+                        0b001'00'0'11'0110011001100'0'0100111,
+                        {1, 2, 3, 4, 5, 6, 7, 0b101'00010}));
+    REQUIRE(subscription != nullptr);  // Subscription exists.
+    REQUIRE(1 == accept(100'002'002,   // Accepted!
+                        0b001'00'0'11'0110011001100'0'0100111,
+                        {'D', 'U', 'C', 'K', 0xD3, 0x14, 0b010'00010}));
+    REQUIRE(subscription != nullptr);  // Subscription exists.
+    REQUIRE(subscription->port_id == 0b0110011001100);
+    REQUIRE(transfer.timestamp_usec == 100'002'001);
+    REQUIRE(transfer.metadata.priority == CanardPriorityImmediate);
+    REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage);
+    REQUIRE(transfer.metadata.port_id == 0b0110011001100);
+    REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
+    REQUIRE(transfer.metadata.transfer_id == 2);
+    REQUIRE(transfer.payload_size == 11);
+    REQUIRE(0 == std::memcmp(transfer.payload,
+                             "\x01\x02\x03\x04\x05\x06\x07"
+                             "DUCK",
+                             11));
+    REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2);  // The SESSION and the PAYLOAD BUFFER.
+    REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
+    REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
+    ins.getAllocator().deallocate(transfer.payload);
+    REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);  // The payload buffer is gone.
+    REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));
+}