Bugzilla – Bug 273
method arguments of type Ptr<Packet> are way too fragile in public API
Last modified: 2008-08-25 17:55:07 EDT
This is a really old never-ending complaint of mine that passing around pointer ownership is _bad_ but I am getting tired of spotting bugs such as this one in csma-net-device.cc: m_rxTrace (packet); if (!m_promiscRxCallback.IsNull ()) { m_promiscRxCallback (this, packet, 0, GetBroadcast (), GetAddress (), PACKET_HOST); } m_rxCallback (this, packet, 0, GetBroadcast ()); There is at least another occurence of this bug in this file: this code assumes that m_promisxRxCallback does not take ownership of the input packet pointer which is wrong because the signature of NetDevice::PromiscReceiveCallback and NetDevice::ReceiveCallback clearly state that the callee takes ownership of the input packet pointer because it is of type Ptr<Packet>. The callee code is actually really taking ownership of the packet so, we did not get any bug report yet but they will eventually come when we use bridging with multiple interfaces and ipv4. I found out about this bug when trying to implement the same functionality in the wifi net device: I was trying to get this right and make the needed packet copies in the caller WifiNetDevice when I noticed that it was incredibly painful and I wondered if the csma code had got it right. It obviously had not. Proposal: use Ptr<const Packet> instead of Ptr<Packet> in: - NetDevice::ReceiveCallback - NetDevice::PromiscReceiveCallback - Node::ProtocolHandler I think that it would also make sense to avoid the same problem in the downward direction by also changing the following methods to use Ptr<const Packet>: - NetDevice::Send - NetDevice::SendFrom Please, can we avoid another endless argument about this and, for once, do the right thing, that is, stay on the safe side of this crap and stop passing around packet pointer ownership across _very_ public API ? patch coming.
I should point out that there is the _exact_ same bug in bridge-net-device.cc.
Created attachment 221 [details] change the upcalls to use const.
Heh, sorry about that. I guess I got used to copy-on-write early on and never got very used to these shared packet pointers semantics :P +1 from me, whatever it's worth.
(In reply to comment #2) > Created an attachment (id=221) [details] > change the upcalls to use const. I changed only the upcalls in this patch because the downcalls are much less problematic and much less potentially subject to bugs so, I guess I could live with them. I think that the crux of the rationale we should use is that, if you have multiple receivers for a packet, you should use const and, if not, you can conceivably, in the name of efficiency, pass a non-const. I personally would just pass const all the time if I had to chose though and that would mean changing Queue::Enqueue and Queue::Dequeue. Until we get to the downcalls, the upcalls look pretty safe to me and do improve quite a bit the situation of NetDevice implementors so, I would like to check them in asap. Comments ?
I have a really hard time with this one since it is such a basic conceptual thing that you don't pass a pointer to multiple consumers if you don't want them both modifying the packet. I also have a hard time with passing a const packet to a method that will almost certainly change it in order to make life easier in the calling method. Unfortunately, it seems that in practice it is just too easy to make this mistake. One of the things I keep saying to myself is, "if we can't get it right, what hope is there for some poor Joe who doesn't work with our code all the time." We addressed the memory management aspects of this and similar situations, but there are other semantic issues remaing that are hidden here. It seems that making the API pass const packets will put up a firewall that will prevent this kind of thing in the future at the costs of looking a little strange and introducing possibly unnecessary packet copies. So I am reluctantly forced to agree with Mathieu at this point. Up and down const packets and take the performance hit of the copy to avoid programming errors; and if that is a problem, optimize the copy. I can't believe I wrote that ...
changeset e5ab96db540e