Bug 572 - Tag offsets not updated in Packet::PeekData ()
Tag offsets not updated in Packet::PeekData ()
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: core
ns-3-dev
All All
: P5 blocker
Assigned To: ns-bugs
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-22 09:23 EDT by Pavel Boyko
Modified: 2009-06-02 13:40 EDT (History)
2 users (show)

See Also:


Attachments
fix for ns-3-dev (596 bytes, patch)
2009-05-22 09:23 EDT, Pavel Boyko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Boyko 2009-05-22 09:23:13 EDT
Created attachment 444 [details]
fix for ns-3-dev

I'm using byte tags from http://code.nsnam.org/mathieu/ns-3-tags, but bug also exist in ns-3-dev. 

The mysterious problem, I have faced, is that PCAP writer attached to promisc sniffer in YansWifiPhy sometimes destroys byte tags tagged on the packets. Unfortunately I can't reproduce this behavior in simple isolated test case, sorry. 

As far as I can see, the reason is that PCAP writer calls harmless Packet::PeekData() which calls harmless Buffer::PeekData() which is, actually, not harmless and non-constant at all -- it can expand virtual payload to real zero bytes if needed. Also Buffer::m_start and m_end offsets can change, in my case of 20 byte payload I see:

Before PeekData(): m_start = 20, m_end = 116, m_zeroAreaStart = 92, m_zeroAreaEnd = 112
After PeekData(): m_start = 0, m_end = 96, m_zeroAreaStart = 92, m_zeroAreaEnd = 92

Since byte tags store offsets in packet buffer (don't ask me why), they must be updated in Packet::PeekData().

Attached patch (adapted from ns-3-tags for ns-3-dev) fixes this, and now PCAP writer doesn't spoil my tags. 

Since buffer, packet and tags implementation still confuses me a lot, I'd like to ask Mathieu to write corresponding unit test if I'm right.
Comment 1 Mathieu Lacage 2009-05-29 10:43:40 EDT
*** Bug 564 has been marked as a duplicate of this bug. ***
Comment 2 Tommaso Pecorella 2009-06-01 06:54:23 EDT
Good one, however the problem still remains somewhere.

I had my hard time to build a test case, and your suggested change completely fix it, however:

1) in my test case no PCAP was called (not a biggie, as something else calls the PeekData) and, most important

2) I have a larger simulation that still shows the wrong behaviour.

Point is, the larger simulation is totally unsuitable as a test case.

Anyway, I've seen that the tag (a time tag in my case) disappear between some header and trailer removals. Ofc it's not a consistent behaviour, 90% of the times all goes fine, 10% of the packets have corrupted tags. The kind of bugs I love.

I'll go search for something similar to the problem you spotted in those functions, but I'd not consider this bug as fixed yet.
Comment 3 Pavel Boyko 2009-06-01 08:01:29 EDT
> Anyway, I've seen that the tag (a time tag in my case) disappear between some
> header and trailer removals. Ofc it's not a consistent behaviour, 90% of the
> times all goes fine, 10% of the packets have corrupted tags. The kind of bugs I
> love.

  This is a symptom of broken buffer offsets stored in the tags, this happens when buffer changes its origin offset. M.b. you have faced with the case, when new buffer start offset is _larger_ than an old one -- I'm afraid that in this case my patch with AddAtStart() doesn't help and you can try AddAtEnd().

> I'll go search for something similar to the problem you spotted in those
> functions, but I'd not consider this bug as fixed yet.

  Good luck!

Comment 4 Mathieu Lacage 2009-06-02 06:26:34 EDT
here is a reduced testcase for bug 572. I can't figure out what is wrong with 564 though.

  {
    // bug 572                                                                                                                   
    Ptr<Packet> tmp = Create<Packet> (1000);
    tmp->AddTag (ATestTag<20> ());
    CHECK (tmp, 1, E (20, 0, 1000));
    tmp->AddHeader (ATestHeader<2> ());
    CHECK (tmp, 1, E (20, 2, 1002));
    tmp->RemoveAtStart (1);
    CHECK (tmp, 1, E (20, 1, 1001));
    tmp->PeekData ();
    CHECK (tmp, 1, E (20, 1, 1001));
  }
Comment 5 Mathieu Lacage 2009-06-02 13:40:39 EDT
changeset e12cbd513b47