Bug #4168: Interest::setNonce affects other instances sharing buffer - ndn-cxx - NDN project issue tracking system

Actions

Copy link

Bug #4168

closed

Interest::setNonce affects other instances sharing buffer

Added by Junxiao Shi over 7 years ago. Updated over 7 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Junxiao Shi

Category:

Base

Target version:

v0.6

Start date:

Due date:

% Done:

100%

Estimated time:

2.00 h

Description

Snippet to reproduce:

// g++ -std=c++11 -o x x.cpp $(pkg-config --cflags --libs libndn-cxx)

#include <ndn-cxx/interest.hpp>
#include <iostream>

int
main()
{
  ndn::Interest i1("/A");
  i1.setNonce(1);
  i1.wireEncode();
  std::cout << i1.getNonce() << ' ';
  ndn::Interest i2(i1);
  std::cout << i2.getNonce() << ' ';
  i2.setNonce(2);
  std::cout << i1.getNonce() << ' ';
  std::cout << i2.getNonce() << '\n';
}

Expected: Output is 1 1 2 1.
Actual: Output is 1 1 2 2.

Actions

Copy link

Updated by Junxiao Shi over 7 years ago

Interest::m_nonce is introduced in order to "make changing Nonce fast", however it causes this nasty problem.
This is not only premature optimization, but also can cause AsfStrategy, the only usage of setNonce within NFD, to react incorrectly to Nacks.

When probing is due, AsfStrategy sends not only a normal Interest to the prefered nexthop from its measurements, but also a probing Interest to another interface. The probing Interest is created by copying the normal Interest and calling refreshNonce on the copy, and thus affected by this bug.
Since all Interest instances share the same wire encoding, the Interest stored on PIT in-record gains the random new Nonce.
In case the node must respond a Nack to the downstream, the Nack would be created with the wrong Nonce. Note that AsfStrategy does not currently send Nacks to downstream when Nacks are received from upstream, so this situation can only happen if strategy is changed to best-route at runtime.
The downstream drops the Nack due to Nonce mismatch.

I think the solution is removing the premature optimization, and just resetting m_wire whenever setNonce is invoked.
Then, m_nonce can be a simple uint32_t quantity, so that getNonce() could be faster.
Also, it is no longer necessary to call wireDecode within wireEncode.

Actions

Copy link

Updated by Alex Afanasyev over 7 years ago

You can call everything "premature optimization". Is it known to cause a problem or just a conjecture? I'm not really in favor of resetting wire, but would not object the change.

Actions

Copy link

Updated by Junxiao Shi over 7 years ago

Assignee set to Junxiao Shi

Is it known to cause a problem or just a conjecture?

The problem can be triggered by following the steps in note-1. It's unlikely to happen in practice, but it would become a hard-to-debug problem if it happens when AsfStrategy is updated to return Nacks.

Actions

Copy link