Bug 1304 - Static deserialize functions (was: Tag information changed after transmission)
Static deserialize functions (was: Tag information changed after transmission)
Status: ASSIGNED
Product: ns-3
Classification: Unclassified
Component: internet
ns-3-dev
All All
: P5 minor
Assigned To: George Riley
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-25 04:10 EST by wangke2000123
Modified: 2012-03-20 16:28 EDT (History)
2 users (show)

See Also:


Attachments
Patch file generated for ipv4-l3-protocol.cc (1.46 KB, patch)
2011-11-25 04:11 EST, wangke2000123
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wangke2000123 2011-11-25 04:10:00 EST
Ipv4PacketInfoTag is attached to every packet sent out and retrieved when received by others. This is done by modifying ipv4-l3-protocol.cc.

In the Ipv4PacketInfoTag, local IP address is recorded. However, when this is retrieved at receiving end, it's always 102.102.102.102. But variables other than Ipv4Addresses seem can be retrieved correctly. From I have tried, Time and Double work fine.
Comment 1 wangke2000123 2011-11-25 04:11:55 EST
Created attachment 1276 [details]
Patch file generated for ipv4-l3-protocol.cc
Comment 2 wangke2000123 2011-12-01 20:24:33 EST
Just run the aodv.cc under AODV example can see the results
Comment 3 Tommaso Pecorella 2012-03-18 16:09:55 EDT
This is a bit more complex than I thought. If you add these lines to Ipv4L3Protocol::Send

  Ipv4PacketInfoTag infoTag;
  if( packet->RemovePacketTag(infoTag) )
    {
      std::cout << "tag was already present " << infoTag.GetAddress() << std::endl;;
    }
  infoTag.SetAddress(GetAddress (1, 0).GetLocal ()); 
  packet->AddPacketTag(infoTag); 
  std::cout << "added " << GetAddress (1, 0).GetLocal () << std::endl;
  std::cout << "added(2) " << infoTag.GetAddress() << std::endl;

  std::cout << "Tags: ";
  packet->PrintPacketTags(std::cout);
  std::cout << std::endl;

 you'll get this:
added 10.0.0.1
added(2) 10.0.0.1
Tags: Ipv4 PKTINFO [DestAddr: 102.102.102.102, Local Address:102.102.102.102, RecvIf:0, TTL:0]  assert failed. cond="item.GetTypeId ().HasConstructor ()", file=../src/network/model/packet.cc, line=871

Now, this is the issue's origin. Mind that Ipv4PacketInfoTag constructor is perfectly fine, so for its TypeId metadata.

I'm not a Tag expert, so I'll leave the resolution to the experts. However I think it's a quite huge bug. and it can be the bell ringing for something bigger.

I think we should raise the priority of this.

T.
Comment 4 wangke2000123 2012-03-20 02:08:29 EDT
Hi,

    For the bug I mentioned,  It's a tiny bug in Deserilize(). 
in file ipv4-packet-info-tag.cc in internet folder. 
in line 124 and 126 
instead of 
  m_addr.Deserialize (buf); 
  m_spec_dst.Deserialize (buf); 
it should be : 
m_addr = Ipv4Address::Deserialize (buf); 
m_spec_dst = Ipv4Address::Deserialize (buf); 

Then it will work fine. however, for your case, I don't know whether it is the same bug..


(In reply to comment #3)
> This is a bit more complex than I thought. If you add these lines to
> Ipv4L3Protocol::Send
> 
>   Ipv4PacketInfoTag infoTag;
>   if( packet->RemovePacketTag(infoTag) )
>     {
>       std::cout << "tag was already present " << infoTag.GetAddress() <<
> std::endl;;
>     }
>   infoTag.SetAddress(GetAddress (1, 0).GetLocal ()); 
>   packet->AddPacketTag(infoTag); 
>   std::cout << "added " << GetAddress (1, 0).GetLocal () << std::endl;
>   std::cout << "added(2) " << infoTag.GetAddress() << std::endl;
> 
>   std::cout << "Tags: ";
>   packet->PrintPacketTags(std::cout);
>   std::cout << std::endl;
> 
>  you'll get this:
> added 10.0.0.1
> added(2) 10.0.0.1
> Tags: Ipv4 PKTINFO [DestAddr: 102.102.102.102, Local Address:102.102.102.102,
> RecvIf:0, TTL:0]  assert failed. cond="item.GetTypeId ().HasConstructor ()",
> file=../src/network/model/packet.cc, line=871
> 
> Now, this is the issue's origin. Mind that Ipv4PacketInfoTag constructor is
> perfectly fine, so for its TypeId metadata.
> 
> I'm not a Tag expert, so I'll leave the resolution to the experts. However I
> think it's a quite huge bug. and it can be the bell ringing for something
> bigger.
> 
> I think we should raise the priority of this.
> 
> T.
Comment 5 Tommaso Pecorella 2012-03-20 11:30:30 EDT
Hi,

you're damn right, it's a bug.

And this raise another issue:
tommaso:ns-3-dev pecos$ grep -r Deserialize src/*/*/*.h | grep static
src/network/utils/ipv4-address.h:  static Ipv4Address Deserialize (const uint8_t buf[4]);
src/network/utils/ipv6-address.h:  static Ipv6Address Deserialize (const uint8_t buf[16]);
src/network/utils/packetbb.h:  static Ptr<PbbMessage> DeserializeMessage (Buffer::Iterator &start);

WHY those 3 Deserializes are static ?!?!?!

I'll check better later tonight, but in my opinion they should NOT be static, as all the other Deserialize(s) are not. So those 3 are a bug source (the user will use them as "normal" but they don't change the instance status.

Design bug.

BTW, the vary same bug (or similar ones) might be scattered around. I'll check those as well and I'll do a global patch.


T.
Comment 6 Tommaso Pecorella 2012-03-20 16:27:35 EDT
Fixed with changeset:   7780 - 819d9c315765

Leaving it open, downgrading the priority and changing the name as I don't like the idea of Deserialize static functions.

T.