NFD » ndn-cxx

Junxiao Shi wrote:

Old code accesses entry after it has been erased, which is undefined behavior.

Are you sure? The code makes a copy of the shared_ptr before erasing the entry...

Unit test to reproduce (face.t.cpp), need to make receiveElement NDN_CXX_PUBLIC_WITH_TESTS_ELSE_PROTECTED in face.hpp:

BOOST_AUTO_TEST_CASE(SatisfyPendingInterestSegfault)
{
  face.expressInterest(Interest("/localhost/notification/1"),
                       [&] (const Interest& i, const Data& d) {
                         std::cout << "Got data from notification stream " << std::endl;
                         std::cout << "Publish into chronosync" << std::endl;
                         face.receive(*makeData("/chronosync/sampleDigest/1"));
                       },
                       bind([] { BOOST_FAIL("Unexpected Nack"); }),
                       bind([] { BOOST_FAIL("Unexpected timeout"); }));

  advanceClocks(10_ms);

  // ChronoSync sets interest filter for sync prefix (/chronosync)
  face.setInterestFilter(ndn::InterestFilter("/chronosync").allowLoopback(false),
    [&] (const InterestFilter&, const Interest& interest) {
      std::cout << "Got interest for ChronoSync" << std::endl;
    });

  advanceClocks(10_ms);

  shared_ptr<Interest> interest = make_shared<Interest>();
  interest->setName("/chronosync/sampleDigest");
  interest->setNonce(1);

  face.expressInterest(*interest,
                       [&] (const Interest& i, const Data& d) {
                         std::cout << "Got data for chronosync" << std::endl;
                       },
                       bind([] { BOOST_FAIL("Unexpected Nack"); }),
                       bind([] { BOOST_FAIL("Unexpected timeout"); }));

  // Same interest but from forwarder (other chronosync)
  interest->setNonce(2);
  face.onReceiveElement(interest->wireEncode());

  advanceClocks(10_ms);

  // Got a notification from subscribed stream
  face.receive(*makeData("/localhost/notification/1"));

  advanceClocks(10_ms);
}

So I guess here is what happens:
1) face has pending interest from app for /localhost/notification/1
2) face has pending interest from app for /chronosync/sampleDigest
3) face has pending interest from fwd for /chronosync/sampleDigest
4) face receives data for /localhost/notification/1 and goes in satisfyPendingInterest and satisfies the pending interest from 1)
5) face publishes data for /chronosync/sampleData and goes in satisfyPendingInterest
6) /chronosync/sampleData is erased from pending interests [2) and 3) both]
7) Loop from 4) continues and tries to get the next entry (/chronoSync/sampleData) which is already erased

In the current fix the two /chronosync/... pending interests are deleted first and then the /localhost/... pending interest is deleted which makes the next i to be end instead of /chronosync/... like before.

I am not sure if this is correct.
Seems like we should not allow satisfyPendingInterests to be called again while it is executing. Any suggestions/comments?

Also with this fix nothing crashes in my experiments but notification subscriber does not get either data or timeout from face (I need to investigate this further as to exactly how it happens).

I'm a bit confused by the test case in note-4. An application would use Face::put() to send Data, not receive(), which is a member of DummyClientFace and is only used because it's a test case.

If this is really a reentrancy issue, then I suppose the fix would be to use post instead of dispatch in Face::put().

With face.put as well the segfault can be reproduced.

Yes, with post there is no segfault. Where to read more about this?

The difference between post and dispatch is explained in Boost.Asio reference doc https://www.boost.org/doc/libs/1_58_0/doc/html/boost_asio/reference/io_service.html

Thanks! So should I post to gerrit?

In the current fix the two /chronosync/... pending interests are deleted first and then the /localhost/... pending interest is deleted which makes the next i to be end instead of /chronosync/... like before.
I am not sure if this is correct.

This old fix is incorrect. It fails in the following case:

1. App expresses Interest /A CanBePrefix=1. Its DataCallback puts Data /A/2.
2. Face receives Data /A/1 from forwarder.
3. Face::Impl::satisfyPendingInterests iterates to PendingInterest /A, and invokes DataCallback.
4. Face::Impl::satisfyPendingInterests iterates to PendingInterest /A, and invokes DataCallback again.
5. Program does not terminate.

If this is really a reentrancy issue, then I suppose the fix would be to use post instead of dispatch in Face::put().

This new fix works:

1. App expresses Interest /A CanBePrefix=1. Its DataCallback puts Data /A/2.
2. Face receives Data /A/1 from forwarder.
3. Face::Impl::satisfyPendingInterests iterates to PendingInterest /A, invokes DataCallback, and erases the PendingInterest.
4. Face::Impl::satisfyPendingInterests finds that there's no PendingInterest, and thus drops the Data.

also, does anyone have a better solution that doesn't require calling post() every time? in the vast majority of cases, dispatch is enough and is more efficient.

One alternative is simply declaring invoking put in DataCallback or NackCallback as undefined behavior. Applications that need this should use face.getIoService().post explicitly.
Another alternative is adding a flag "is satisfyPendingInterest running". Face::put always calls dispatch (it cannot read the flag because Face::put is meant to be thread safe). Face::Impl::satisfyPendingInterests posts itself if the flag is set.

Junxiao Shi wrote:

One alternative is simply declaring invoking put in DataCallback or NackCallback as undefined behavior. Applications that need this should use face.getIoService().post explicitly.

Yes, but this "solution" is less convenient for application developers. Plus, are we going to audit all existing code?

Another alternative is adding a flag "is satisfyPendingInterest running". Face::put always calls dispatch (it cannot read the flag because Face::put is meant to be thread safe). Face::Impl::satisfyPendingInterests posts itself if the flag is set.

It may not be just satisfyPendingInterest, other internal functions might not expect to be called recursively.