To reproduce this bug, you can run this command:
 NS_LOG='UdpEchoServerApplication:UdpEchoClientApplication'
./build/debug/examples/udp-echo

which will print this:
2000000000ns UdpEchoClientApplication:Send(): Sent 1024 bytes to 10.1.1.2
2007899600ns UdpEchoServerApplication:HandleRead(): Received 1024 bytes from
10.1.1.1
2007899600ns UdpEchoServerApplication:HandleRead(): Echoing packet
2015800200ns UdpEchoClientApplication:HandleRead(0x630bf0, 0x63d9f0)
2015800200ns UdpEchoClientApplication:HandleRead(): Received 1024 bytes from
10.1.1.1


The bug lies in UdpEchoClientApplication:HandleRead() where we observe a packet
coming from 10.1.1.1 while it is really coming from 10.1.1.2.

The issue here is not entirely trivial. What happens is that the packet is
tagged with the address 10.1.1.1 when reaching the server application and, is
then returned to the client where it is tagged a second time with the address
10.1.1.2. The key issue is that the logging code uses
Packet::FindFirstMatchingTag to find the tagging address but this call will
find the "first matching tag", that is the tag stored when the packet was
received on the server.

The only reasonable option seems to be to call Packet::RemoveAllTags in the
server before sending back the packet to the client. Patch attached.

Created an attachment (id=179) [details]
fix bug

> The only reasonable option seems to be to call Packet::RemoveAllTags in the
> server before sending back the packet to the client. Patch attached.

Hmmm.  This doesn't sound like a very good solution to me.  The root of the
problem is in the socket code, not the echo server code.  I don't think it's a
good plan to have the echo server just strip off all of the tags because the
socket code doesn't deal with the tags it adds properly.

I would be very surprised to find that some random application just decided to
delete all the tags I very carefully added in my simulation.

*The* problem is that (I'll use UDP) ForwardUp creates and attaches a
SocketAddressTag to a packet and queues the packet.  RecvFrom dequeues the
packet and does a FindFirstMatchingTag but leaves the tag there.  If the packet
is forwarded to another socket, the now useless tag is left there and results
in the next ForwardUp adding a duplicate tag and RecvFrom getting the wrong
one.

No, I don't think the UDP echo server application is the right place to fix
this.  I think that ForwardUp needs to error out if it finds a SocketAddressTag
in a packet.  I think it needs to add one.  I think RecvFrom needs to find that
single tag, use it to get the address and then remove it.  

This means adding the ability to remove a tag to a packet.  It seems to me that
the simplest thing would be a RemoveFirstMatchingTag call.

(In reply to comment #3)
> > The only reasonable option seems to be to call Packet::RemoveAllTags in the
> > server before sending back the packet to the client. Patch attached.
> 
> Hmmm.  This doesn't sound like a very good solution to me.  The root of the
> problem is in the socket code, not the echo server code.  I don't think it's a
> good plan to have the echo server just strip off all of the tags because the
> socket code doesn't deal with the tags it adds properly.
> 
> I would be very surprised to find that some random application just decided to
> delete all the tags I very carefully added in my simulation.
> 
> *The* problem is that (I'll use UDP) ForwardUp creates and attaches a
> SocketAddressTag to a packet and queues the packet.  RecvFrom dequeues the
> packet and does a FindFirstMatchingTag but leaves the tag there.  If the packet
> is forwarded to another socket, the now useless tag is left there and results
> in the next ForwardUp adding a duplicate tag and RecvFrom getting the wrong
> one.
> 
> No, I don't think the UDP echo server application is the right place to fix
> this.  I think that ForwardUp needs to error out if it finds a SocketAddressTag
> in a packet.  I think it needs to add one.  I think RecvFrom needs to find that
> single tag, use it to get the address and then remove it. 

I agree that if the semantics of a particular tag are that it is consumed by
another layer, it should be OK to remove it in that different layer.

However, this may be prone to some error because someone may write a new
application and forget to remove the tag.  Or, maybe someone doesn't use
RecvFrom() but Recv() instead and is unaware of the presence of that tag.  So
maybe ForwardUp should just also remove that tag if it happens to already be
there, rather than error out.

> 
> This means adding the ability to remove a tag to a packet.  It seems to me that
> the simplest thing would be a RemoveFirstMatchingTag call.
> 

I would support this.  In addition, another use case is if people try to use
tags for hop-by-hop stuff, they will need the ability to remove tags in
transit.

However, I think it might also be a good idea to consider RemoveAllTags in the
application, in addition to trying to make the code clean up this particular
tag.  This is perhaps a policy decision we should discuss further (whether
Packets, when consumed by an application, are considered to be logically dead
and if one wants to reuse them to send back down the stack again, the default
behavior may be to flush all tags before doing that).  Because I think it is
debatable which will be more surprising-- that a recycled packet contains tags
from a previous communication or not.

The symptom of the bug is fixed, but there is more here.  I will reduce the
priority to P3, but leave the bug to address the underlying problem with tag
usage and lack of a remove method.  I have renamed the bug and reduced the
severity to normal as well.

This bug could now be fixed relatively trivially since we have a
RemovePacketTag method which could be used in UdpSocketImpl::ForwardUp

updating title to more accurately reflect status of work left