Bug 231 - SocketAddressTag needs to be removed from a packet before forwarding the packet to the user
SocketAddressTag needs to be removed from a packet before forwarding the pack...
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: internet
pre-release
All All
: P4 normal
Assigned To: Tom Henderson
:
Depends on: 2419
Blocks: 2308
  Show dependency treegraph
 
Reported: 2008-06-23 15:54 EDT by Craig Dowell
Modified: 2016-06-01 19:18 EDT (History)
9 users (show)

See Also:


Attachments
fix bug (544 bytes, patch)
2008-06-24 15:30 EDT, Mathieu Lacage
Details | Diff
remove all SocketAddressTag before adding another (662 bytes, patch)
2012-02-19 16:43 EST, Tom Henderson
Details | Diff
alternative patch without using SocketAddressTag (2.69 KB, patch)
2012-02-23 07:12 EST, Nicola Baldo
Details | Diff
replace SocketAddressTag if already present (5.60 KB, patch)
2016-05-19 15:19 EDT, Tom Henderson
Details | Diff
UdpSocketImpl simplification (3.20 KB, patch)
2016-05-21 18:57 EDT, Tommaso Pecorella
Details | Diff
patch - removes PacketAddressTag (610.95 KB, patch)
2016-05-24 18:01 EDT, Tommaso Pecorella
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Craig Dowell 2008-06-23 15:54:15 EDT
The UDP echo application now reports that an echo response comes from the IP address of the interface on which the client resides and not the interface that actually did the echo.
Comment 1 Mathieu Lacage 2008-06-24 15:29:26 EDT
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.
Comment 2 Mathieu Lacage 2008-06-24 15:30:09 EDT
Created attachment 179 [details]
fix bug
Comment 3 Craig Dowell 2008-06-24 17:49:39 EDT
> 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.
Comment 4 Tom Henderson 2008-06-25 00:50:30 EDT
(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.
Comment 5 Craig Dowell 2008-06-27 12:37:09 EDT
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.
Comment 6 Mathieu Lacage 2009-11-23 08:48:37 EST
This bug could now be fixed relatively trivially since we have a RemovePacketTag method which could be used in UdpSocketImpl::ForwardUp
Comment 7 Mathieu Lacage 2009-11-23 08:50:00 EST
updating title to more accurately reflect status of work left
Comment 8 Tom Henderson 2010-04-16 18:44:22 EDT
I revisited this today and found that the attached patch has been added somewhere along the way.

UdpEchoServer::HandleRead (Ptr<Socket> socket)
{
  Ptr<Packet> packet;
  Address from;
  while (packet = socket->RecvFrom (from))
    {
      if (InetSocketAddress::IsMatchingType (from))
        {
          InetSocketAddress address = InetSocketAddress::ConvertFrom (from);
          NS_LOG_INFO ("Received " << packet->GetSize() << " bytes from " <<
            address.GetIpv4());

          packet->RemoveAllPacketTags ();
          packet->RemoveAllByteTags ();

          NS_LOG_LOGIC ("Echoing packet");
          socket->SendTo (packet, 0, from);
        }
    }
}


Will mark fixed unless more info is provided.
Comment 9 Tom Henderson 2010-07-16 00:12:41 EDT
marking as resolved since there were no additional comments
Comment 10 Nicola Baldo 2011-10-19 12:52:14 EDT
I am experiencing this bug again. I have a setup where UDP/IP packets are tunneled over UDP/IP. In this setup, the same packets passes twice though UdpSocketImpl::ForwardUp, and since the SocketAddressTag added by the UDP socket of the tunnel is never removed, I get an error when the end-user UDP socket tries to add another tag of the same type on the same packet.

I agree with the previous posts that the patch on UdpEchoServer fixed the symptom but not the problem. I can easily put a similar workaround in my code, but I wonder if there is any better solution...
Comment 11 Mathieu Lacage 2011-10-19 14:57:23 EDT
(In reply to comment #10)
> I am experiencing this bug again. I have a setup where UDP/IP packets are
> tunneled over UDP/IP. In this setup, the same packets passes twice though
> UdpSocketImpl::ForwardUp, and since the SocketAddressTag added by the UDP
> socket of the tunnel is never removed, I get an error when the end-user UDP
> socket tries to add another tag of the same type on the same packet.
> 
> I agree with the previous posts that the patch on UdpEchoServer fixed the
> symptom but not the problem. I can easily put a similar workaround in my code,
> but I wonder if there is any better solution...

My feeling is that if you intend to forward a packet, you should make a copy before handing it back down the stack and the copy should be stripped of all tags.
Comment 12 Tom Henderson 2012-02-19 16:43:20 EST
Created attachment 1335 [details]
remove all SocketAddressTag before adding another

If this patch is acceptable, should do similar to TCP and NSC-TCP.
Comment 13 Tommaso Pecorella 2012-02-20 12:59:28 EST
The patch seems fine, however I have a question.

From what I got about the issue:
- it seems perfectly legal to add two tags of the identical type. It's up to the Tag Reader and Tag writer to prove and maintain tags uniqueness (when needed).

From a design point of view, shouldn't it better to add a property to the tags "unique", so to (if the tag is marked unique:
- when the tag is added, discard or overwrite the old one.

This approach would fix to the boots the issue, also for future tags. Moreover is more foolproof. The cons is a slightly increased overhead when adding a tag.

Cheers,

T.
Comment 14 Nicola Baldo 2012-02-23 07:12:57 EST
Created attachment 1337 [details]
alternative patch without using SocketAddressTag


I think the proposed patch is not 100% satisfactory because, in the case that the application uses the RecvCallback instead of calling UdpSocketImpl::Recv(), the SocketAddressTag would still be left there.

Additionally, I think the use of SocketAddressTag in this case is not necessary and could easily be avoided. Please find attached an alternative patch that does this.
Comment 15 Tom Henderson 2012-02-29 21:45:06 EST
(In reply to comment #14)
> Created attachment 1337 [details]
> alternative patch without using SocketAddressTag
> 
> 
> I think the proposed patch is not 100% satisfactory because, in the case that
> the application uses the RecvCallback instead of calling UdpSocketImpl::Recv(),
> the SocketAddressTag would still be left there.
> 
> Additionally, I think the use of SocketAddressTag in this case is not necessary
> and could easily be avoided. Please find attached an alternative patch that
> does this.

I like your patch better.  If no other comments, I propose to patch it like you suggest.
Comment 16 Tom Henderson 2012-03-05 00:43:37 EST
(In reply to comment #14)

> 
> Additionally, I think the use of SocketAddressTag in this case is not necessary
> and could easily be avoided. Please find attached an alternative patch that
> does this.

Does anyone have a concern about removing this tag as per the suggested patch (and could also be removed from the socket.h API)?  This is an implicit API change, in case someone is using this feature.
Comment 17 Tommaso Pecorella 2012-03-05 07:14:07 EST
I have no concerns about removing the SocketAddressTag altogether, however the proposed patch is only involving Udp if I'm right.

The SocketAddressTag, however, is used in some other places. Excluding the obvious socket.cc / .h, the tag is used here:

tommaso:ns-3-dev pecos$ grep -r SocketAddressTag src/*/model/*
src/internet/model/nsc-tcp-socket-impl.cc:      SocketAddressTag tag;
src/internet/model/nsc-tcp-socket-impl.cc:  SocketAddressTag tag;
src/internet/model/tcp-socket-base.cc:      SocketAddressTag tag;
src/internet/model/udp-socket-impl.cc:      SocketAddressTag tag;
src/internet/model/udp-socket-impl.cc:      SocketAddressTag tag;
src/internet/model/udp-socket-impl.cc:      SocketAddressTag tag;
examples/routing/manet-routing-compare.cc:  SocketAddressTag tag;

Plus I know people is using it to track UDP packets incoming interface (some mails in ns-3 Users list).

I'm not at all against changing the API if the new solution is more flexible and closer to the real world, however we should be consistent and, if we remove it from UDP, we shall do the same for all the other L4 implementations.

Cheers,

T.
Comment 18 Mathieu Lacage 2012-03-05 09:11:47 EST
frederic, does this impact dce ?
Comment 19 Tom Henderson 2012-03-05 09:22:55 EST
(In reply to comment #17)
> I have no concerns about removing the SocketAddressTag altogether, however the
> proposed patch is only involving Udp if I'm right.
> 
> The SocketAddressTag, however, is used in some other places. Excluding the
> obvious socket.cc / .h, the tag is used here:
> 
> tommaso:ns-3-dev pecos$ grep -r SocketAddressTag src/*/model/*
> src/internet/model/nsc-tcp-socket-impl.cc:      SocketAddressTag tag;
> src/internet/model/nsc-tcp-socket-impl.cc:  SocketAddressTag tag;
> src/internet/model/tcp-socket-base.cc:      SocketAddressTag tag;
> src/internet/model/udp-socket-impl.cc:      SocketAddressTag tag;
> src/internet/model/udp-socket-impl.cc:      SocketAddressTag tag;
> src/internet/model/udp-socket-impl.cc:      SocketAddressTag tag;
> examples/routing/manet-routing-compare.cc:  SocketAddressTag tag;

Yes, and once we remove it from these, then it could be removed from socket.h
because there are no more uses for it.

> 
> Plus I know people is using it to track UDP packets incoming interface (some
> mails in ns-3 Users list).

This is what I am talking about.  I was going to try to progess this patch, but
then I hesitated, thinking that maybe we are removing something that people
will complain about when their code no longer works.

I think that the current functionality could be replaced by asking users to use
RecvFrom when they need the source address, but I can't recall why we
introduced this tag in the first place.
Comment 20 Frederic Urbani 2012-03-05 10:19:47 EST
(In reply to comment #18)
> frederic, does this impact dce ?

Yes, DCE use this tag setted in packet-socket.cc in order to try to implement
 socket of domain AF_PACKET and type SOCK_RAW,

so I am better agree of removing SocketAddressTag elsewhere then in src/network/utils/packet-socket.cc source file.
Comment 21 Tommaso Pecorella 2012-03-05 10:57:57 EST
PacketAddressTag was (and is) used to get the informations that should be in the real structures (e.g., struct sockaddr).

No matter what the consequences will be, including but not limited to:
- existing code backward compatibility breaks,
- changes to actual code and derivates (e.g., dce) and so on,
- etcetera
my personal opinion is that at some point you have to improve the APIs to get more flexibility and a closer behavior to the real systems.

At the end of the day, maybe the new APIs will be way simpler to use than the old ones, and all will be happy. However, if we block improvements to keep backward compatibility, we'll not improve (tautology).

So, the right question is not if PacketAddressTag is used (it is). The right question is:
** will the new API contain the same informations? If so, where they will be stored? **

Once this is clear the patch can be merged with:
- warnings about the new API and
- how to migrate existing code.

Cheers,

T.
Comment 22 Frederic Urbani 2012-03-05 11:01:22 EST
(In reply to comment #21)
> PacketAddressTag was (and is) used to get the informations that should be in
> the real structures (e.g., struct sockaddr).
> 
> No matter what the consequences will be, including but not limited to:
> - existing code backward compatibility breaks,
> - changes to actual code and derivates (e.g., dce) and so on,
> - etcetera
> my personal opinion is that at some point you have to improve the APIs to get
> more flexibility and a closer behavior to the real systems.
> 
> At the end of the day, maybe the new APIs will be way simpler to use than the
> old ones, and all will be happy. However, if we block improvements to keep
> backward compatibility, we'll not improve (tautology).
> 
> So, the right question is not if PacketAddressTag is used (it is). The right
> question is:
> ** will the new API contain the same informations? If so, where they will be
> stored? **
> 
> Once this is clear the patch can be merged with:
> - warnings about the new API and
> - how to migrate existing code.
> 
> Cheers,
> 
> T.

I am agree with you.
Comment 23 Mathieu Lacage 2012-03-06 03:03:42 EST
frederic, my question, was: can dce deal with the proposed patch ?
Comment 24 Frederic Urbani 2012-03-06 04:06:01 EST
(In reply to comment #23)
> frederic, my question, was: can dce deal with the proposed patch ?

Yes, the patch number 1337 is ok for DCE. But I am still agree with Tommaso.
Comment 25 Tom Henderson 2016-05-19 15:19:35 EDT
Created attachment 2440 [details]
replace SocketAddressTag if already present
Comment 26 Tom Henderson 2016-05-19 15:24:06 EDT
The tentative bug 2308 solution is causing the issue raised by Nicola in comment 10 to reappear.  I posted a new patch that I believe can close the issue.  From the commit message:

SocketAddressTag is added to Packet objects as a packet tag by transport
protocols.  Packet tags of the same type cannot be added twice to the
same Packet.  In certain tunneling situations (e.g. UDP tunneled over
UDP), the packet may traverse the UDP receive chain twice before
being delivered to an application, triggering an assert.  The issue
can be resolved by using Packet::ReplacePacketTag() instead of
Packet::AddPacketTag() in transport protocols, as ReplacePacketTag
will overwrite any pre-existing tag of the same type without error,
or fall back to the semantics of AddPacketTag.  This also seems to
preserve the intended semantics in a tunneling situation, since the
application will be more concerned with the inner (and not outer,
tunnelled) address (see RFC 1853 sec. 2 for inner/outer definition).

---

I think this is more appropriate now than Nicola's solution (to avoid using the tag altogether) because we have "leaked" this tag across our API for many years now, and removing it may break other examples and protocols (e.g. RIP).  So, I propose that we instead document in the TCP/UDP/PacketSocket class doxygen that this tag is part of the Recv() interface (attached patch carries such suggested Doxygen).
Comment 27 natale.patriciello 2016-05-19 16:25:32 EDT
I'm reading through all the older comments, and what I miss is why "SocketAddressTag is added to Packet objects as a packet tag by transport protocols." .. there isn't the API RecvFrom for this information?

From the sources it seems that RecvFrom get the address from this Tag.. so I wonder why, in first instance, the API was written to make extensive use of this tag. Isn't better to try to avoid the use of PacketTag in our code, giving the problems we have (and the performance cost of serializing and deserializing, of course) ? It involves rewriting the API, so it will be a pain, but maybe is something worth the effort..
Comment 28 Tom Henderson 2016-05-19 17:53:43 EDT
(In reply to natale.patriciello from comment #27)
> I'm reading through all the older comments, and what I miss is why
> "SocketAddressTag is added to Packet objects as a packet tag by transport
> protocols." .. there isn't the API RecvFrom for this information?
> 
> From the sources it seems that RecvFrom get the address from this Tag.. so I
> wonder why, in first instance, the API was written to make extensive use of
> this tag. Isn't better to try to avoid the use of PacketTag in our code,
> giving the problems we have (and the performance cost of serializing and
> deserializing, of course) ? It involves rewriting the API, so it will be a
> pain, but maybe is something worth the effort..

Your point is valid that this duplicates RecvFrom() behavior.  My proposal is based on assumption that we have gone six or more years with this and it is fixable without breaking any API.  

On the other hand, I don't remember the history of why this tag usage started to be included in the internet transport protocols, so if maintainer sentiment favors instead to scrub our transport sockets of this tag, and fix the existing usages to RecvFrom(), I could be convinced.
Comment 29 natale.patriciello 2016-05-20 05:16:46 EDT
(In reply to Tom Henderson from comment #28)
> Your point is valid that this duplicates RecvFrom() behavior.  My proposal
> is based on assumption that we have gone six or more years with this and it
> is fixable without breaking any API.  
> 
> On the other hand, I don't remember the history of why this tag usage
> started to be included in the internet transport protocols, so if maintainer
> sentiment favors instead to scrub our transport sockets of this tag, and fix
> the existing usages to RecvFrom(), I could be convinced.

Of course, changing the API is a mid-to-long term proposal; right now, I agree with the patch.
Comment 30 Tommaso Pecorella 2016-05-20 09:56:23 EDT
(In reply to natale.patriciello from comment #29)
> (In reply to Tom Henderson from comment #28)
> > Your point is valid that this duplicates RecvFrom() behavior.  My proposal
> > is based on assumption that we have gone six or more years with this and it
> > is fixable without breaking any API.  
> > 
> > On the other hand, I don't remember the history of why this tag usage
> > started to be included in the internet transport protocols, so if maintainer
> > sentiment favors instead to scrub our transport sockets of this tag, and fix
> > the existing usages to RecvFrom(), I could be convinced.
> 
> Of course, changing the API is a mid-to-long term proposal; right now, I
> agree with the patch.

I disagree on one thing: it's not a mid-long term solution.

My proposal is:
1) apply the patch,
2) change RIP (I don't even know why I was so lazy to use that) and 
3) declare SocketAddressTag obsolete.
4) remove it altogether in 3.27.

In the patch, tho, there are a coupe of things to fix.
- " * \see class SocketAddresssTag" <- one "s" too much, and it's repeated in two places.
- We can point out already that SocketAddressTag is not to be used, as its functionality is in RecvFrom, and for TCP it's clearly a non-existent issue.
Comment 31 Tommaso Pecorella 2016-05-21 09:59:16 EDT
Changeset 12131:fcefcf4aa600 revived the RIP dependency on SocketAddressTag.

Now, just to summarize my understanding: SocketAddressTag is a bug and it must be removed.

SocketAddressTag purpose is to enable RecvFrom, it wasn't meant to be passed up to other layers. Its leakage to upper layers is the bug. Moreover, it's unnecessary to use a Tag in all the cases where SocketAddressTag is used.

SocketAddressTag is used in 3 places: UdpSocketImpl, TcpSocketBase and NscTcpSocketImp. 

- In TcpSocketBase it is plainly unnecessary. if the user wants to know the sender's address he/she must use TcpSocketBase::GetPeerName.

- In NscTcpSocketImp it is used in the wrong way. Moreover, (again) GetPeerName.

- In UdpSocketImpl it is used in the must "correct" way, i.e., to tag an incoming packet with its sender address (there's a Rx queue in UdpSocketImpl). Still, we don't need SocketAddressTag. We just need to use a pair <Packet, Address> and queue that.

As a consequence, we can safely remove SocketAddressTag from all the codebase without impairments... with the exception of those that did use Recv instead of RecvFrom.
Comment 32 Tommaso Pecorella 2016-05-21 18:57:21 EDT
Created attachment 2448 [details]
UdpSocketImpl simplification

This doesn't modify anything (yet), and it still carries AddPacketTag instead of ReplacePacketTag.

However, it goes in the direction of removing SocketAddressTag altogether. The tag is added just before Recv / RecvFrom, and UdpSocket is not anymore dependent on its presence.

No matter how we fix this problem (and I agree on using ReplacePacketTag), I feel that this patch is beneficial.
It also removes an useless procedure on RecvFrom, as the previous system was:
1) Ip sends up a packet - add a Tag
2) The user calls RecvFrom
3) RecvFrom calls Recv
4) RecvFrom **peeks the tag** and returns the packet.
with the new approach, there's no tag peeking.

As a side note: in Tom's patch there is a[nother] small glitch: SocketAddressTag can carry a InetSocketAddress *or* a Inet6SocketAddress.
Comment 33 Tom Henderson 2016-05-24 14:10:28 EDT
(In reply to Tommaso Pecorella from comment #31)

> 
> As a consequence, we can safely remove SocketAddressTag from all the
> codebase without impairments... with the exception of those that did use
> Recv instead of RecvFrom.

I am fine with this solution; let's just document in CHANGES.html that any users who relied on Recv() and the tag should now change their code to use RecvFrom ().
Comment 34 Tommaso Pecorella 2016-05-24 18:01:25 EDT
Created attachment 2452 [details]
patch - removes PacketAddressTag

Global patch. It removes PacketAddressTag altogether from the codebase.
All the bindings needed to be refreshed, making the patch huge.
Comment 35 Tommaso Pecorella 2016-06-01 19:18:15 EDT
pushed in changeset:   12139:6869e7e72137