pcap: trust computed IEEE 802.11 FCS metadata

Avoid serializing every captured MPDU and recomputing its FCS when the typed trailer is marked FCS_COMPUTED. Standard INET capture paths calculate that trailer after the final MAC fields are set, so the adapter now treats it as present and trusted while continuing to map FCS_DECLARED_INCORRECT to Radiotap BADFCS.

Document the capture design decisions raised during review: typed-chunk requirements for FCS and A-MPDU recognition, aggregate padding and whole-PSDU fallback, Radiotap field layout, adapter registry uniqueness and lifecycle, protocol-adapter record semantics, wireless observation fallback, and PCAPng interface and snaplen handling.

Update the Radiotap FCS unit coverage so a deliberately mismatching computed value remains trusted, and remove the obsolete serialization-failure fixture. Verified with the release build and the focused PCAP adapter, A-MPDU, registry, and writer-prefix unit tests.

Author: mgonzalezlopezudc

src/inet/common/packet/recorder/PcapCaptureAdapterRegistry.cc

@@ -16,6 +16,8 @@ PcapCaptureAdapterRegistry::~PcapCaptureAdapterRegistry()
         delete entry.second;
 }
 
+// Each protocol, resolver key, and observation key has one owner per network setup. Silently
+// replacing an entry would make capture behavior depend on registration order, so conflicts fail fast.
 void PcapCaptureAdapterRegistry::registerProtocolAdapter(const Protocol *protocol, const IPcapCaptureAdapter *adapter)
 {
     if (protocol == nullptr || adapter == nullptr || protocolAdapters.find(protocol) != protocolAdapters.end()) {
@@ -71,6 +73,8 @@ std::optional<PcapCaptureObservation> PcapCaptureAdapterRegistry::tryCreateObser
 
 PcapCaptureAdapterRegistry& PcapCaptureAdapterRegistry::getInstance()
 {
+    // SharedDataManager scopes the registry to the current network lifecycle, allowing the
+    // pre-network registration fragments to run again after the previous network is deleted.
     static int handle = cSimulationOrSharedDataManager::registerSharedVariableName("inet::PcapCaptureAdapterRegistry::instance");
     return getSimulationOrSharedDataManager()->getSharedVariable<PcapCaptureAdapterRegistry>(handle);
 }

src/inet/common/packet/recorder/PcapRecorder.cc

@@ -180,6 +180,8 @@ void PcapRecorder::receiveSignal(cComponent *source, simsignal_t signalID, cObje
         auto observation = PcapCaptureAdapterRegistry::getInstance().tryCreateObservation(obj, direction);
         if (observation.has_value())
             recordPacket(*observation, source);
+        // Observation adapters are optional enrichers. If none accepts the object, retain the
+        // generic cPacket path; non-INET packet payloads eventually remain unrecorded as before.
         else if (auto packet = dynamic_cast<const cPacket *>(obj))
             recordPacket(packet, direction, source);
     }
@@ -191,13 +193,19 @@ void PcapRecorder::writePacket(const Protocol *protocol, const PcapCaptureObserv
     if (enableProtocolSpecificCaptureAdapters && enableConvertingPackets) {
         auto adapter = PcapCaptureAdapterRegistry::getInstance().findProtocolAdapter(protocol);
         if (adapter != nullptr) {
+            // A protocol adapter owns its output link type and complete record layout, so its
+            // records bypass the generic link-type matching and packet-conversion helpers below.
             auto records = adapter->createRecords(observation, frontOffset, backOffset);
             for (const auto& record : records) {
                 auto dataLength = packet->getDataLength() - record.frontOffset - record.backOffset;
+                // A protocol-specific prefix is meaningful capture data, so a prefix-only record
+                // is not considered empty even when recordEmptyPackets is false.
                 if (recordEmptyPackets || !record.getPrefix().empty() || dataLength != b(0)) {
                     pcapWriter->writePacketWithPrefix(simTime(), record.getPrefix(), packet, record.frontOffset, record.backOffset,
                             observation.direction, networkInterface, adapter->getLinkType());
                     numRecorded++;
+                    // Emit once per written record, but retain the original observed packet as the
+                    // signal value; split records such as A-MPDU MPDUs therefore share that value.
                     emit(packetRecordedSignal, packet);
                 }
             }

src/inet/common/packet/recorder/PcapngWriter.cc

@@ -209,6 +209,8 @@ void PcapngWriter::writePacketWithPrefix(simtime_t stime, const std::vector<uint
     if (!dumpfile)
         throw cRuntimeError("Cannot write frame: pcap output file is not open");
 
+    // Enhanced Packet Blocks refer to an Interface Description Block, unlike classic PCAP
+    // records. Fail explicitly when no interface can be resolved instead of dereferencing null.
     if (networkInterface == nullptr)
         throw cRuntimeError("The interface entry not found for packet");
 
@@ -224,6 +226,8 @@ void PcapngWriter::writePacketWithPrefix(simtime_t stime, const std::vector<uint
 
     b packetLength = packet->getDataLength() - frontOffset - backOffset;
     size_t originalLength = prefix.size() + packetLength.get<B>();
+    // Advertise and enforce the configured snaplen for PCAPng too. The captured length is
+    // truncated, while originalPacketLength below retains the complete untruncated record length.
     size_t capturedLength = std::min<size_t>(originalLength, snaplen);
     uint32_t optionsLength = (4 + 4) + 4;
     uint32_t blockTotalLength = 32 + roundUp(capturedLength) + optionsLength;

src/inet/linklayer/ieee80211/pcap/Ieee80211RadiotapPcapCaptureAdapter.cc

@@ -14,8 +14,6 @@
 
 #include "inet/common/INETMath.h"
 #include "inet/common/ProtocolTag_m.h"
-#include "inet/common/checksum/Checksum.h"
-#include "inet/common/packet/chunk/BytesChunk.h"
 #include "inet/common/packet/recorder/PcapCaptureAdapterRegistry.h"
 #include "inet/linklayer/ieee80211/mac/Ieee80211Frame_m.h"
 #include "inet/physicallayer/wireless/common/contract/packetlevel/INarrowbandSignalAnalogModel.h"
@@ -164,6 +162,8 @@ AmpduParseResult getIeee80211AmpduMpduRanges(const Packet *packet, b frontOffset
     if (frontOffset + ieee80211::LENGTH_A_MPDU_SUBFRAME_HEADER > endOffset)
         return AmpduParseResult::NOT_AGGREGATE;
 
+    // Delimiters are recognized only as typed chunks at their exact boundaries. A serialized
+    // BytesChunk is deliberately not guessed to be an aggregate; it is captured as one PSDU below.
     auto peekDelimiter = [&] (b offset) {
         return dynamicPtrCast<const ieee80211::Ieee80211MpduSubframeHeader>(packet->peekDataAt(offset, b(-1), parsingFlags));
     };
@@ -193,6 +193,7 @@ AmpduParseResult getIeee80211AmpduMpduRanges(const Packet *packet, b frontOffset
             if (offset == endOffset)
                 return AmpduParseResult::VALID;
             auto paddingLength = B((4 - (delimiter->getChunkLength() + mpduLength).get<B>() % 4) % 4);
+            // This mirrors MpduAggregation::aggregateFrames(): pad between MPDUs, but not after the last one.
             // IEEE 802.11-2024, 9.7.1 and 10.12.6 permit exact final-subframe alignment padding for VHT/HE-family PPDUs.
             // Without PHY-mode provenance, accept the structurally complete equality case instead of discarding its MPDUs.
             if (offset + paddingLength > endOffset)
@@ -219,6 +220,8 @@ FcsMetadata getIeee80211FcsMetadata(const Packet *packet, b frontOffset, b backO
         return {};
     FcsMetadata metadata;
     try {
+        // A typed trailer is authoritative evidence that the final four octets are an FCS. For a
+        // raw BytesChunk they may instead be payload, so the adapter does not infer FCS presence.
         auto trailer = dynamicPtrCast<const ieee80211::Ieee80211MacTrailer>(packet->peekDataAt(endOffset - B(4), B(4)));
         if (trailer == nullptr)
             return metadata;
@@ -227,11 +230,11 @@ FcsMetadata getIeee80211FcsMetadata(const Packet *packet, b frontOffset, b backO
             case FCS_DECLARED_INCORRECT:
                 metadata.isBad = true;
                 break;
-            case FCS_COMPUTED: {
-                auto data = packet->peekDataAt<BytesChunk>(frontOffset, endOffset - frontOffset - trailer->getChunkLength());
-                metadata.isBad = ethernetFcs(data->getBytes()) != trailer->getFcs();
+            case FCS_COMPUTED:
+                // On standard INET capture paths, a typed FCS_COMPUTED trailer was produced by INET
+                // after the final MAC fields were set. Trust it instead of serializing the MPDU and
+                // repeating the linear-time FCS calculation solely for packet capture.
                 break;
-            }
             case FCS_DECLARED_CORRECT:
             default:
                 break;
@@ -338,6 +341,8 @@ RadiotapPpduFields extractRadiotapPpduFields(const Packet *packet, Direction dir
 
 std::vector<uint8_t> serializeRadiotapHeader(const RadiotapPpduFields& fields, const RadiotapRecordMetadata& metadata)
 {
+    // Fields are appended in increasing present-bit order. Padding is relative to the beginning
+    // of this buffer, which already contains the fixed eight-octet Radiotap header.
     uint32_t present = 0;
     auto setPresentBit = [&] (RadiotapPresentBit bit) { present |= 1U << bit; };
     std::vector<uint8_t> bytes(8, 0);
@@ -482,6 +487,8 @@ std::vector<PcapCaptureRecord> Ieee80211RadiotapPcapCaptureAdapter::createRecord
         return records;
     }
 
+    // Malformed aggregates and delimiter-only input still represent an observed wireless frame.
+    // Preserve it as one whole-PSDU record instead of silently producing no capture records.
     RadiotapRecordMetadata metadata;
     auto fcsMetadata = getIeee80211FcsMetadata(packet, frontOffset, backOffset);
     metadata.hasFcs = fcsMetadata.isPresent;

src/inet/physicallayer/wireless/common/pcap/WirelessPcapCaptureObservationAdapter.cc

@@ -20,6 +20,8 @@ std::optional<PcapCaptureObservation> WirelessPcapCaptureObservationAdapter::try
 {
     if (auto signal = dynamic_cast<const Signal *>(object)) {
         auto packet = dynamic_cast<const Packet *>(signal->getEncapsulatedPacket());
+        // Returning nullopt keeps the recorder's generic cPacket fallback available. If the
+        // encapsulated object is not an INET Packet, that fallback ignores it as before.
         return packet != nullptr ? std::optional<PcapCaptureObservation>(PcapCaptureObservation(packet, direction)) : std::nullopt;
     }
     else if (auto transmission = dynamic_cast<const ITransmission *>(object))

tests/unit/PcapRecorderRadiotapHtVht_1.test

@@ -3,7 +3,6 @@ Test legacy Rate, HT MCS, VHT, and FCS Radiotap fields without fabricating unsup
 
 %includes:
 #include "inet/common/packet/chunk/BytesChunk.h"
-#include "inet/common/packet/chunk/FieldsChunk.h"
 #include "inet/common/ProtocolTag_m.h"
 #include "inet/common/checksum/Checksum.h"
 #include "inet/linklayer/ieee80211/mac/Ieee80211Frame_m.h"
@@ -22,16 +21,6 @@ using namespace inet::physicallayer;
 
 #define REQUIRE(...) do { if (!(__VA_ARGS__)) throw cRuntimeError("REQUIRE failed at line %d: %s", __LINE__, #__VA_ARGS__); } while (false)
 
-class UnserializableFieldsChunk : public FieldsChunk
-{
-  public:
-    UnserializableFieldsChunk() { setChunkLength(B(4)); }
-    UnserializableFieldsChunk(const UnserializableFieldsChunk& other) = default;
-
-    virtual UnserializableFieldsChunk *dup() const override { return new UnserializableFieldsChunk(*this); }
-    virtual const Ptr<Chunk> dupShared() const override { return makeShared<UnserializableFieldsChunk>(*this); }
-};
-
 static uint16_t readUint16(const std::vector<uint8_t>& bytes, size_t offset)
 {
     return bytes.at(offset) | bytes.at(offset + 1) << 8;
@@ -157,7 +146,7 @@ mismatchedTrailer->setFcsMode(FCS_COMPUTED);
 mismatchedTrailer->setFcs(ethernetFcs(fcsPayload) ^ 1);
 computedMismatch.insertAtBack(mismatchedTrailer);
 auto computedMismatchHeader = createRadiotapHeader(adapter, computedMismatch);
-REQUIRE(computedMismatchHeader.at(8) == 0x50); // typed FCS is present and authoritatively mismatches
+REQUIRE(computedMismatchHeader.at(8) == 0x10); // computed FCS is present and trusted without verification
 
 Packet genericBitError("genericBitError");
 genericBitError.insertAtBack(makeShared<BytesChunk>(fcsPayload));
@@ -177,14 +166,6 @@ declaredIncorrect.insertAtBack(incorrectTrailer);
 auto declaredIncorrectHeader = createRadiotapHeader(adapter, declaredIncorrect);
 REQUIRE(declaredIncorrectHeader.at(8) == 0x50);
 
-Packet unserializablePayload("unserializablePayload");
-unserializablePayload.insertAtBack(makeShared<UnserializableFieldsChunk>());
-auto unserializablePayloadTrailer = makeShared<Ieee80211MacTrailer>();
-unserializablePayloadTrailer->setFcsMode(FCS_COMPUTED);
-unserializablePayload.insertAtBack(unserializablePayloadTrailer);
-auto unserializablePayloadHeader = createRadiotapHeader(adapter, unserializablePayload);
-REQUIRE(unserializablePayloadHeader.at(8) == 0x10); // typed FCS remains present when payload comparison fails
-
 EV << "Legacy, HT, VHT, and FCS Radiotap fields tested successfully.\n";
 
 %contains: stdout