Bugzilla – Bug 2078
Packet metadata disabling on a per-packet basis
Last modified: 2016-05-25 20:49:14 EDT
Some packets may not have metadata, and still be perfectly valid.
One example is in Icmpv6L4Protocol::HandleDestinationUnreachable.
The packet being carried by the ICMP is "rebuilt" from a byte buffer, and it doesn't contain any metadata.
If the user enables metadata checking ("Packet::EnableChecking ();"), any ICMPv6 Destination Unreachable will cause a crash.
One solution (without adding flags to the packet class) is to add a new RemoveHeader (and RemoveTrailer) functions, explicitly stating that metadata are not to be checked.
Created attachment 1991 [details]
Test script (it could be reduced to "send an Icmpv6 Destination Unreachable")
Created attachment 1992 [details]
This patch disables metadata removal from headers or trailers if the packet has been created from a plain byte buffer.
I still don't know why the
Packet::Packet (uint8_t const*buffer, uint32_t size)
constructor implicitly creates a "size" fake header metadata. Removing it is harmful.
I tried the patch on ns-3-dev and while I found that it fixed the example program, it caused other example/test failures.
./waf --run examples/ipv6/icmpv6-redirect
'build' finished successfully (4.052s)
Node: 1, Time: +0.0s, Local time: +0.0s, Ipv6StaticRouting table
Destination Next Hop Flag Met Ref Use If
::1/128 :: UH 0 - - 0
fe80::/64 :: U 0 - - 1
2001:1::/64 :: U 0 - - 1
2001:2::200:ff:fe00:5/128 fe80::200:ff:fe00:3 UH 0 - - 1
assert failed. cond="m_current >= delta", file=./ns3/buffer.h, line=859
terminate called without an active exception
These are the new failures due to the patch I will attach.
List of FAILed tests:
List of CRASHed tests:
Created attachment 2300 [details]
revised patch for ns-3-dev tip
Created attachment 2306 [details]
Changed the approach to much, much better one.
The developer have to *explicitly* mark the packet as non-metadata safe.
The previous idea was not considering the case of payloads built by a buffer with metadata-safe headers.
Note that there's still one problem, as is what to do with the case of mix-n-match.
Example: IP packet built from a buffer (non-metadata-safe). Then you add the Ethernet header / trailer. those *will* add metadata.
Upon removing the Ethernet header/trailer, metadata are not removed, because the packet is not safe.
So, the question is: shall we remove all the metadata if the packet is not safe ?
What to do when two packets are merged (AddAtEnd) and only one of them is non-metadata-safe ?
The actual patch fixes an actual bug, but the above points could come back sooner or later.
(In reply to Tommaso Pecorella from comment #5)
> Created attachment 2306 [details]
> New patch
> Changed the approach to much, much better one.
> The developer have to *explicitly* mark the packet as non-metadata safe.
> The previous idea was not considering the case of payloads built by a buffer
> with metadata-safe headers.
What about making m_metadataDisabled an instance member of PacketMetadata instead, and then the only touch to class Packet is:
* Disable, on a per-packet basis, the use of metadata. This must be used
* if metadata checking is globally enabled, and if the packet is built in a
* manner (e.g. from a raw byte buffer) such that the metadata is
('Disabled' seems a clearer term than 'Dirty' to me)
(also, document that the Create<Packet> () method of packet creation from
raw byte buffer will disable metadata even if it is globally enabled)
> Note that there's still one problem, as is what to do with the case of
> Example: IP packet built from a buffer (non-metadata-safe). Then you add the
> Ethernet header / trailer. those *will* add metadata.
> Upon removing the Ethernet header/trailer, metadata are not removed, because
> the packet is not safe.
> So, the question is: shall we remove all the metadata if the packet is not
> safe ?
Suggest to disable if this doesn't cause problems elsewhere (i.e. prevent mix-n-match from occurring).
> What to do when two packets are merged (AddAtEnd) and only one of them is
> non-metadata-safe ?
Suggest to disable and document this as part of AddAtEnd().
> The actual patch fixes an actual bug, but the above points could come back
> sooner or later.
Created attachment 2456 [details]
Trying again. This time the changes are (almost) only to PacketMetadata.
If the approach is ok, the only thing missing is to modify the metadata tests to check this new functionality.