Bug #4180
closed
Block::fromStream consumes extra octet when TLV-LENGTH equals zero
Added by Junxiao Shi almost 7 years ago.
Updated almost 7 years ago.
Description
Snippet to reproduce:
// g++ -std=c++11 -o x x.cpp $(pkg-config --cflags --libs libndn-cxx)
#include <ndn-cxx/encoding/block.hpp>
#include <sstream>
#include <iostream>
int
main()
{
std::istringstream is(std::string("\x01\x00\x03\x02\xee\xff", 6));
ndn::Block b1 = ndn::Block::fromStream(is);
std::cout << b1.type() << ' ' << b1.size() << std::endl;
ndn::Block b2 = ndn::Block::fromStream(is);
std::cout << b2.type() << ' ' << b2.size() << std::endl;
}
The input is:
TLV-TYPE=1 TLV-LENGTH=0 TLV-VALUE={}
TLV-TYPE=3 TLV-LENGTH=2 TLV-VALUE={0xee, 0xff}
Expected:
1 2
3 4
Actual:
1 2
terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<ndn::tlv::Error> >'
what(): Not enough data in the buffer to fully parse TLV
Aborted (core dumped)
When decoding the first TLV element, one extra octet has been consumed from the input stream, causing decoding error at the second TLV element.
- Related to Bug #2729: Block::fromStream decode error when TLV-LENGTH equals zero added
- Assignee set to Junxiao Shi
My solution is:
- Introduce a new
incrementMode argument to tlv::readVarNumber function.
Its default value is IncrementMode::AFTER which increments the begin iterator to one octet past the VAR-NUMBER.
Its alternate value is IncrementMode::LAST which increments the begin iterator to the last octet of the VAR-NUMBER.
- Specify
IncrementMode::LAST when reading TLV-LENGTH in Block::fromStream, so that no extra octet would be consumed.
This also eliminates the special case of dereferencing the iterator as the first octet of TLV-VALUE.
I'll code this after Change 4017 and Change 4014 and Change 4030 are merged.
I notice that Block::fromStream is not used in ndn-cxx or NFD. Is it still worth fixing, or should I just remove this function?
It is needed for ndnSIM integration.
- Status changed from New to In Progress
I see. It's in BlockHeader::Deserialize and there's no obvious alternative.
On the other hand, that use case only deals with network-layer packets, so it is impossible for TLV-LENGTH to be zero.
Given that fixing this bug involves adding a parameter and conditional to tlv::readVarNumber which would slightly worsen performance, I'd throw an exception when TLV-LENGTH is zero instead.
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
you expect to crash simulations if MustBeFresh selector is used?
It won't. An ns3::Packet always carries a full Interest, Data, or LpPacket, where the top-level TLV element has non-zero TLV-LENGTH. BlockHeader (and therefore Block::fromStream) is only used to extract the top-level TLV element. Sub-elements such as MustBeFresh are decoded with Block::decode which does not invoke this code path.
I would be ok with this then, unless istream::putback can be easily used to simply address the issue.
- Status changed from Code review to Feedback
istream::putback can be easily used to simply address the issue
Yes, putback is much better. It also eliminates the special case of first octet of TLV-VALUE.
However, can you confirm it works for ns3::Buffer::Iterator?
- Status changed from Feedback to Code review
- Status changed from Code review to Closed
Also available in: Atom
PDF
Updated by 
Updated by