Project

General

Profile

Actions

Bug #4726

closed

Block class mishandles type == 0xffffffff (was: NFD crashes on unexpected input)

Added by price clark about 6 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Category:
Base
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

Description

STEPS TO REPRODUCE:

  • Start an instance of NFD
  • run the command echo -e '\xfe\xff\xff\xff\xff\x00' | nc 127.0.0.1 6363

EXPECTED:

  • NFD does not crash

OBSERVED:

  • NFD crashes

Related issues 1 (0 open1 closed)

Related to NDN Specifications - Feature #4895: Limit TLV-TYPE rangeClosedJunxiao Shi

Actions
Actions #1

Updated by price clark about 6 years ago

https://github.com/peetonn/ndn-docker/tree/master/node

was using this build so this happened using the bleeding edge NFD/ndn-cxx.

Actions #2

Updated by Davide Pesavento about 6 years ago

  • Start date deleted (08/30/2018)

Can you provide a gdb backtrace obtained with "bt full"?

Actions #3

Updated by Davide Pesavento about 6 years ago

  • run the command `nc -z -v

This command is incomplete.

Actions #4

Updated by Davide Pesavento about 6 years ago

  • Status changed from New to Rejected

I've tried to reproduce using various combinations of nc options, and NFD never crashed. Critical info is missing from the bug report, so it's impossible to debug further. Rejecting.

Actions #5

Updated by price clark over 5 years ago

echo -e '\xfe\xff\xff\xff\xff\x00' | nc 127.0.0.1 6363

courtesy of Sam McKay

Actions #6

Updated by Junxiao Shi over 5 years ago

  • Description updated (diff)
  • Status changed from Rejected to New

I'm able to reproduce this bug in NFD 0.6.4.
Crash happens on UDP as well.

Actions #7

Updated by Davide Pesavento over 5 years ago

With git master branch:

1553024644.794751 TRACE: [nfd.TcpChannel] [tcp4://0.0.0.0:6363] Incoming connection from 127.0.0.1:46642
1553024644.794877 INFO: [nfd.TcpTransport] [id=0,local=tcp4://127.0.0.1:6363,remote=tcp4://127.0.0.1:46642] Creating transport
1553024644.794906 INFO: [nfd.FaceTable] Added face id=261 remote=tcp4://127.0.0.1:46642 local=tcp4://127.0.0.1:6363
1553024644.795208 TRACE: [nfd.TcpTransport] [id=261,local=tcp4://127.0.0.1:6363,remote=tcp4://127.0.0.1:46642] Received: 7 bytes
1553024644.795278 FATAL: [nfd.Main] ../ndn-cxx/encoding/block.cpp(292): Throw in function size
Dynamic exception type: boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<ndn::Block::Error> >
std::exception::what: Block size cannot be determined (undefined block size)
===== Stacktrace =====
 0# ndn::Block::size() const at ../ndn-cxx/encoding/block.cpp:292
 1# nfd::face::StreamTransport<boost::asio::ip::tcp>::handleReceive(boost::system::error_code const&, unsigned long) at ../daemon/face/stream-transport.hpp:255
 2# _ZN5boost4asio6detail23reactive_socket_recv_opINS0_17mutable_buffers_1EZN3nfd4face15StreamTransportINS0_2ip3tcpEE12startReceiveEvEUlDpOT_E_E11do_completeEPvPNS1_19scheduler_operationERKNS_6system10error_codeEm at /usr/include/boost/asio/detail/reactive_socket_recv_op.hpp:122
 3# boost::asio::detail::scheduler::run(boost::system::error_code&) at /usr/include/boost/asio/detail/impl/scheduler.ipp:154
 4# nfd::NfdRunner::run() at ../daemon/main.cpp:155
 5# main at ../daemon/main.cpp:335
 6# 0x00007F3D450BA09B in /lib/x86_64-linux-gnu/libc.so.6
 7# 0x000055ED4AF08D0A in ./build/bin/nfd
======================
Actions #8

Updated by Davide Pesavento over 5 years ago

  • Project changed from NFD to ndn-cxx
  • Subject changed from NFD crashes when it gets unexpected input to NFD crashes on unexpected input
  • Category set to Base

price clark wrote:

echo -e '\xfe\xff\xff\xff\xff\x00' | nc 127.0.0.1 6363

Parsed as TLV, the above blob has a type == 0xffffffff, i.e. the max value for a uint32_t. The ndn::Block class uses this value as sentinel to indicate an invalid TLV block, so subsequently calling size() on such an object (as NFD's StreamTransport does) throws an exception.

You can trigger the same problem with Block{std::numeric_limits<uint32_t>::max()}.size().

I actually noticed this some time ago and thought about using an optional<uint32_t> instead of a special sentinel value for the type, but didn't think it was important.

Actions #9

Updated by Davide Pesavento over 5 years ago

  • Subject changed from NFD crashes on unexpected input to Block class mishandles type == 0xffffffff (was: NFD crashes on unexpected input)
Actions #10

Updated by Davide Pesavento over 5 years ago

There is the related question of what is the maximum TLV-TYPE allowed by the packet format, the spec is unclear on that. We'll discuss this matter tomorrow at the platform call if there's time.

Actions #11

Updated by Junxiao Shi over 5 years ago

what is the maximum TLV-TYPE allowed by the packet format

Packet format is pretty clear to me: minimum 0, maximum 264 -1, defined via the range of VAR-NUMBER.
ndn-cxx chooses to support 32-bit only. The obvious solution is for ndn-cxx to declare it supports [0, 232 -2] only and drop others.

Actions #12

Updated by Davide Pesavento over 5 years ago

Junxiao Shi wrote:

what is the maximum TLV-TYPE allowed by the packet format

Packet format is pretty clear to me: minimum 0, maximum 264 -1, defined via the range of VAR-NUMBER.

I think the general sentiment is that such a large number space for TLV types is unnecessary. The question is whether this should be reflected in the spec.

ndn-cxx chooses to support 32-bit only. The obvious solution is for ndn-cxx to declare it supports [0, 232 -2] only and drop others.

A related question is whether an implementation is allowed to support only a subset of numbers and still be considered compliant. I don't think that's the case given the current wording regarding evolvability: http://named-data.net/doc/NDN-packet-spec/current/tlv.html#considerations-for-evolvability-of-tlv-based-encoding

Actions #13

Updated by Junxiao Shi over 5 years ago

Actions #14

Updated by Davide Pesavento over 5 years ago

  • Status changed from New to Code review
  • Assignee set to Davide Pesavento
  • Target version set to v0.7
  • % Done changed from 0 to 100
Actions #15

Updated by Davide Pesavento over 5 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF