Improve encapsulation of logic relating to Layer attachment to Packet. #2004
Conversation
… to a Layer's ownership from regular fields.
| // Should this really always delete m_Data? What if the layer is attached to a packet? | ||
| if (m_Data != nullptr) | ||
| delete[] m_Data; |
There was a problem hiding this comment.
See comment? This would cause an error if called on a Layer that does not own its data span.
| else | ||
| { | ||
| layer->m_Packet = nullptr; | ||
| layer->m_AllocationInfo.detach(); |
There was a problem hiding this comment.
This line in particular created a subtle bug in the old implementation.
If a layer is originally owned by the packet instance, and detached, the m_IsAllocatedInPacket flag was not zeroed. If the layer has then been attached to another Packet. The attached layer was considered as if managed by the new packet even if attached with addLayer(layer, false) which should not transfer ownership of the layer object.
| /// | ||
| /// If 'false', the Layer object is considered unmanaged and the user is responsible for freeing it. | ||
| /// This is commonly the case for layers created on the stack and attached to a Packet. | ||
| bool managedByPacket = false; |
There was a problem hiding this comment.
Maybe ownedByPacket is a more descriptive name? 🤔
| PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanEthLayer, true)); | ||
| PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanIP4Layer, true)); | ||
| PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanIcmpLayer, true)); |
There was a problem hiding this comment.
Or should we keep the test as is, but correctly delete the layers afterwards?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #2004 +/- ##
==========================================
- Coverage 83.46% 83.45% -0.01%
==========================================
Files 311 311
Lines 54568 54593 +25
Branches 11808 11851 +43
==========================================
+ Hits 45545 45561 +16
+ Misses 8313 8208 -105
- Partials 710 824 +114
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Dimi1010 I'm not sure why replacing 2 protected members ( |
@seladb It mitigates the risk of this issue #2004 (comment) which was a bug that can be hard to track and easy to happen and completely broke the memory ownership model. Having them in the same struct indicates that the two variables are much more tightly related compared to let's say, If the allocation info needs to be updated, it is more likely to be done through behaviour actions ( IMO, this is easier to understand and less bug-prone at the point of use, than having to remember to flip N variables manually every time the operation needs to be performed.
Protected data make it a pain to ensure invariants in the base classes of an inheritance hierarchy. That is especially true for large hierarchies like If it was Making it private with RO accessors (if RO access is needed), changes the situation from "It can happen, but shouldn't." to "It can't happen". Removing the need to scan every code change for if a bug of such nature was introduced. PS: |
There are very few places that need to update these members, and most (if not all) of them reside in the
I agree that layers shouldn't deal with the allocation logic, and I'm pretty sure none of them do. In practice, they never need to unless there's a very special use case. I guess we can leave the RO method although I don't think it's a real pain for people writing protocol parsers - I don't recall a case where I asked someone not to change these members... |
It wouldn't have been that simple to find. I found it only after I grouped the variables and a test suddenly started memory leaking due to being incorrect. It was attaching the layer without ownership and then not deleting the layer. Everything still appeared to run fine because the packet deleted it erroneously. Also, IMO that is only treating the symptom. In memory ownership schemes, generally keeping things simple at the point of use is key to prevent errors. Having to manually remember which variables to flip is not simple at the point of use, even if its only a few places. Having the complex actions encapsulated in methods is better due to explicit requirement for all parameters to be provided before executing the action.
In practice, if the fields aren't needed by the derived classes then they shouldn't be exposed to them. I don't think we will have any special use cases where a derived class will need to change its owned packet from its internals. The only need they have for the packet is to forward the pointer to the next layer which is RO operation. |
You're saying the bug wasn't exposed until you refactored the code and wrote a specific test? It's a good sign that it's an edge case that would rarely happen. We have many protocols using this infrastructure, and they all work fine. That's another indication that this refactoring is not needed - "if it ain't broken don't fix it" 😄
I don't disagree with you, but I don't think it's "painful" enough that we need to modify this code |
@seladb I am saying that the old test was written wrong and did not catch the bug in memory semantics. The bug still existed. The thing that happened was that the refactoring fixed the bug in the Layer's attach/detach procedure and the test started failing because it had another bug of not cleaning up the layers it detached. It was a classic case of 2 bugs covering each other in that particular situation. This is the test case: https://github.com/seladb/PcapPlusPlus/pull/2004/files#diff-995c851e4dd7dc72ed1ba9d8bf87403a50d00e69f92d31cfcab69577b42763a1L310-R332 // The layers are detached from the packet here.
pcpp::EthLayer* vxlanEthLayer = (pcpp::EthLayer*)vxlanPacketOrig.detachLayer(pcpp::Ethernet, 1);
pcpp::IcmpLayer* vxlanIcmpLayer = (pcpp::IcmpLayer*)vxlanPacketOrig.detachLayer(pcpp::ICMP);
pcpp::IPv4Layer* vxlanIP4Layer = (pcpp::IPv4Layer*)vxlanPacketOrig.detachLayer(pcpp::IPv4, 1);
/* Assertion code irrelevant to the issue */
/* [...] */
// This is the test old code. The commented false is because that is the default value for take ownership.
// See how we instruct the attach operation to NOT take ownership of the layer when attaching it again.
PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanEthLayer, /* false */));
PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanIP4Layer, /* false */));
PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanIcmpLayer, /* false */));This should cause the the test to memory leak, because the layers aren't cleaned up explicitly in the test afterwards. Instead what happened was that the test runs fine without memory leaks, because the layer's ownership gets erroneously transferred anyway, because the flag
This leads to the flag keeping its old value of In a correctly written test case that properly handled the cleanup of the layers, it would have lead to double free bug. This attach / detach bug is very easily reproduced by having:
Here is the sample reproducing the memory ownership bug. Run this on the current int main(int argc, char* argv[])
{
using namespace pcpp;
Packet* pkt = new Packet();
Layer* layer = new ArpLayer(ArpReply(MacAddress(), IPv4Address(), MacAddress(), IPv4Address()));
// Attach the layer to the packet, ownership should transfer to the packet
pkt->addLayer(layer, true);
std::cout << "This should be true:" << layer->m_IsAllocatedInPacket << std::endl;
// Detach the layer from the packet, ownership should transfer to the user
pkt->detachLayer(layer);
std::cout << "This should be false:" << layer->m_IsAllocatedInPacket << std::endl;
// Attach the layer to the packet, ownership should NOT transfer to the packet
pkt->addLayer(layer, false);
std::cout << "This should be false:" << layer->m_IsAllocatedInPacket << std::endl;
// Delete packet. In the erogenous case this will also delete the layer object.
delete pkt;
// In the correct case, this should work, as the layer should still be alive.
// In the erogenous case. This will dereference a dangling pointer and crash.
layer->m_IsAllocatedInPacket;
// This should be done by the user since ownership was never transferred back to the packet
// In the erogenous case, it will cause a double free and crash.
delete layer;
return 0;
}This entire issue is why I prefer my ownership mechanics to be somewhat encapsulated into simple objects / operations then relying on flipping flags manually. It might be a somewhat uncommon case of detaching and reattaching a layer, but IMO it is unacceptable behaviour as it breaks the memory ownership model which is of critical importance. |
2 participants
Summary
This PR intended to improve the encapsulation and clarity of Layer field data. As per the C++ core guidelines
protecteddata fields are generally undesired as they represent difficulty in maintaining base class invariants across derived classes.The main object of this PR is the introduction of a new helper struct
LayerAllocationInfoin the internal namespace, to hold allocation information for Layer objects and provide behavior helper methods. The helper structure replaces is contained as a field in aLayerto represent its current allocation state.LayerAllocationInfoand chosen fields.The information structure contains 2 members:
Packet* attachedPacket, replacingm_Packetpointer insideLayer.bool managedByPacket, replacingm_isAllocatedInPacketflag insideLayer.The replacements were chosen due to their relevancy to the allocation behaviour of
Layerand little else. Those fields were previously intermixed with other fields in theLayerobject causing confusion when determining their use and scope.attachedPacket(previouslym_Packet) is exclusively used for indication if the layer's data is owned by a packet and to forward requests for structural data changes to the underlying packet buffer, if available. The new name was chosen to better represent that use.managedByPacket(previouslym_IsAllocatedInPacket) is exclusively used as flag to determine if theLayerobject's lifecycle is managed by thePacketinstance it is attached to. The field intricately tied to the state ofattachedPacketand as such is included to the helper structure.The new fields include extensive documentation regarding their use, improving the maintainability of the code. ( Think about the you in 5 months reading it 🙂 )
Behaviour methods instead of direct field mutation
The new helper structure adds two behavior methods:
attachPacket(Packet* packet, bool managed, bool force = false): This method encapsulates the behavior of attaching a layer to a Packet instance. It allows aPacketinstance attaching aLayerto itself to update the information in one step instead of manually having to directly updateprivatefields to theLayerinstance. The method also provides integrated sanity checks if aLayeris being attached to aPacketinstance twice.detach(): Encapsulates the behaviour of detaching aLayerfromPacket. It allows aPacketinstance detaching aLayerto inform it of its new state in one step.Protected data accessors and private fields
The new
LayerAllocationInfostructure is held as a private field insideLayer. The change improves data integrity as the allocation information (packet or managed flag) is unavailable for accidental modification by derived classes.To keep existing behaviour of a
Layerinstance forwarding its attachment to any other instance it constructs during parsing, a new protected accessorgetAttachedPacket()has been added and instances of direct use ofm_Packetare replaced by it.Bug Fixes
m_IsAllocatedInPacketflag when detaching a layer. If said layer is attached to a new packet instance, the old implementation disregarded the ownership flag and considered the layer instance as owned by the packet, due to the layer flag being set from the previous packet.