Project

General

Profile

Actions
Copy link

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.

Status:
Closed
Priority:
Normal
Assignee:
Junxiao Shi
Category:
Base
Target version:
v0.6
Start date:
07/10/2017
Due date:
% Done:

100%

Estimated time:
1.50 h

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 issues 1 (0 open — 1 closed)

Related to ndn-cxx - Bug #2729: Block::fromStream decode error when TLV-LENGTH equals zeroClosedJunxiao Shi04/04/2015

Actions
Actions
Copy link
 #1

Updated by Junxiao Shi almost 7 years ago

  • Related to Bug #2729: Block::fromStream decode error when TLV-LENGTH equals zero added
Actions
Copy link
 #2

Updated by Junxiao Shi almost 7 years ago

  • Assignee set to Junxiao Shi

My solution is:

  1. 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.
  2. 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.

Actions
Copy link
 #3

Updated by Junxiao Shi almost 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?

Actions
Copy link
 #4

Updated by Alex Afanasyev almost 7 years ago

It is needed for ndnSIM integration.

Actions
Copy link
 #5

Updated by Junxiao Shi almost 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.

Actions
Copy link
 #6

Updated by Junxiao Shi almost 7 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 0 to 100
Actions
Copy link
 #7

Updated by Junxiao Shi almost 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.

Actions
Copy link
 #8

Updated by Alex Afanasyev almost 7 years ago

I would be ok with this then, unless istream::putback can be easily used to simply address the issue.

Actions
Copy link
 #9

Updated by Junxiao Shi almost 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?

Actions
Copy link
 #10

Updated by Junxiao Shi almost 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.

Actions
Copy link
 #11

Updated by Junxiao Shi almost 7 years ago

  • Status changed from Code review to Closed
Actions
Copy link

Also available in: Atom PDF