Bugzilla – Bug 2556
packets can get corrupted while transmitted using CsmaNetDevice
Last modified: 2017-06-03 11:30:33 UTC
Created attachment 2676 [details] proposed patch during the time period CsmaNetDevice transmits a packet, when TransmitStart() schedules TransmitCompleteEvent(), i have checked a packet get corrupted when there is 'low activity' in the CsmaChannel. specifically, packets do not get altered if TransmitCompleteEvent() runs immediately after TransmitStart() i.e. tEvent = 0 in TransmitStart(). here is a short log, where a packet->GetSize() shows a modification in packet length for the same packet UID -and pointer address-. CsmaNetDevice:TransmitStart() m_currentPkt = 0x7fd6f7517980 UID = 8 CsmaNetDevice:IsSendEnabled() CsmaChannel:TransmitStart(0x7fd6f7706080, 0x7fd6f7517980, 0) UID is 8) size is 86) switch to TRANSMITTING size = 86 m_bps = 5000000 Schedule TransmitCompleteEvent in 0.0001376sec CsmaNetDevice:TransmitCompleteEvent() m_currentPkt=0x7fd6f7517980 Pkt UID is 8) Pkt size is 58) CsmaChannel:TransmitEnd(0x7fd6f7706080, 0x7fd6f7517980, 0) UID is 8) size is 58) Schedule event in 0.002 sec i propose a minimal modification not considering CsmaNetDevice::Send() definition because of inheritance.
I am curious; do you know what accounts for adding two bytes? I agree that in general, a copy is needed either within the channel or before handing the packet to the channel.
Would be possible to attach a simulation script inducing this error? I got concerned about this and I would like to help.
hi Tom, (In reply to Tom Henderson from comment #1) > I am curious; do you know what accounts for adding two bytes? i think i catch the meaning of your comment. i agree making local copies for parameters passed to functions is not desirable approach. > I agree that in general, a copy is needed either within the channel or > before handing the packet to the channel. i guess this could get into a philosophical discussion, should the lower layer take care of data passed from an upper one or the later must be responsible. i tend to the first approach, but that is just my view. anyway, as i think i am the only one that have came across this issue for years, i fell this not as critical as i previously thought. i will try to solve at an upper layer. thanks, regards.
hi Luciano, (In reply to Luciano from comment #2) > Would be possible to attach a simulation script inducing this error? I got > concerned about this and I would like to help. i thank you for your helpfulness. this error came up under development of a ns-3 new module, so it could be messy to reproduce it using the regular ns-3 software. i reiterate my gratitudes, regards.
(In reply to francisco javier sanchez-roselly from comment #3) > hi Tom, > > (In reply to Tom Henderson from comment #1) > > I am curious; do you know what accounts for adding two bytes? > > i think i catch the meaning of your comment. i agree making local copies for > parameters passed to functions is not desirable approac On the contrary, I believe that a copy is necessary in the situation of transmitting on a multi-access channel. I had a look at the code and could not spot how a problem might arise at the moment, but I can see how it would arise if one were to make some modifications to CsmaNetDevice. Hence I was curious whether the existing code, or modified code, was causing the problem. I had a situation during some Wi-Fi development in the past where I was not making a copy of a packet on a channel. One receiving station would receive the packet, and remove a header, then a second receiving station would receive the same packet (pointer), and assert because there was no header. So I can see how this can cause problems; I just think we should patch it one way or another even if the existing code does not cause issues. Usually this is handled in the code base by passing Ptr<const Packet> somewhere along the path and forcing the recipient to make a copy.
hi Tom, (In reply to Tom Henderson from comment #5) > (In reply to francisco javier sanchez-roselly from comment #3) > > hi Tom, > > > > (In reply to Tom Henderson from comment #1) > > > I am curious; do you know what accounts for adding two bytes? > > > > i think i catch the meaning of your comment. i agree making local copies for > > parameters passed to functions is not desirable approac > > On the contrary, I believe that a copy is necessary in the situation of > transmitting on a multi-access channel. I had a look at the code and could > not spot how a problem might arise at the moment, but I can see how it would > arise if one were to make some modifications to CsmaNetDevice. Hence I > was curious whether the existing code, or modified code, was causing the > problem. in my code the situation is like this: if an IPv6 node has multiple interfaces, it must send the same ICMPv6 message using the particular interface link-local address for each one. my strategy is: create the ICMPv6 message, add custom IPv6 header for the interface, pass message to lower layer, REMOVE IPv6 header, and so on; in believing the lower layer takes care -e.g. makes a local copy- of the upper layer data. it is not an issue to generate a specific IPv6 packet on every interface, if L2 expects an stable message. the point is this situation has arisen in my simulations since ns-3.25, more or less, so i was puzzled about this sudden odd. thanks, regards. > I had a situation during some Wi-Fi development in the past where I was not > making a copy of a packet on a channel. One receiving station would receive > the packet, and remove a header, then a second receiving station would > receive the same packet (pointer), and assert because there was no header. > So I can see how this can cause problems; I just think we should patch it > one way or another even if the existing code does not cause issues. > > Usually this is handled in the code base by passing Ptr<const Packet> > somewhere along the path and forcing the recipient to make a copy.
Fixed in changeset 12910:6b6e52a125f9 by making TransmitStart take a Ptr<const Packet>; made similar change to point-to-point module in changeset 12911:b0bdf7b395db