Bug #4180
closed
Block::fromStream consumes extra octet when TLV-LENGTH equals zero
100%
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.
Updated by Junxiao Shi over 7 years ago
- Related to Bug #2729: Block::fromStream decode error when TLV-LENGTH equals zero added
Updated by Junxiao Shi over 7 years ago
- Assignee set to Junxiao Shi
My solution is:
- Introduce a new
incrementModeargument totlv::readVarNumberfunction. Its default value isIncrementMode::AFTERwhich increments thebeginiterator to one octet past the VAR-NUMBER. Its alternate value isIncrementMode::LASTwhich increments thebeginiterator to the last octet of the VAR-NUMBER. - Specify
IncrementMode::LASTwhen reading TLV-LENGTH inBlock::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.
Updated by Junxiao Shi over 7 years ago
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?
Updated by Junxiao Shi over 7 years ago
- 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.
Updated by Junxiao Shi over 7 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
Updated by Junxiao Shi over 7 years ago
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.
Updated by Alex Afanasyev over 7 years ago
I would be ok with this then, unless istream::putback can be easily used to simply address the issue.
Updated by Junxiao Shi over 7 years ago
- 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?
Updated by Junxiao Shi over 7 years ago
- Status changed from Feedback to Code review
https://gerrit.named-data.net/4049 patchset4 implements Block::fromStream with putback which handles TLV-LENGTH=0 correctly.
Updated by Junxiao Shi over 7 years ago
- Status changed from Code review to Closed