face: retain PendingInterest during Interest processing
When an incoming Interest matches multiple InterestFilters,
if one of them replies immediately, the PendingInterest
instance could be prematurely destructed and cause memory
access errors. This commit retains the PendingInterest with
a shared_ptr to avoid this bug.
refs #4228
Change-Id: I4767f294711edda3b75f8dfcac085ede88f3cdef
diff --git a/src/detail/face-impl.hpp b/src/detail/face-impl.hpp
index 57ee911..f0c7231 100644
--- a/src/detail/face-impl.hpp
+++ b/src/detail/face-impl.hpp
@@ -100,9 +100,9 @@
addFieldFromTag<lp::NextHopFaceIdField, lp::NextHopFaceIdTag>(lpPacket, interest2);
addFieldFromTag<lp::CongestionMarkField, lp::CongestionMarkTag>(lpPacket, interest2);
+ entry.recordForwarding();
m_face.m_transport->send(finishEncoding(std::move(lpPacket), interest2.wireEncode(),
'I', interest2.getName()));
- entry.recordForwarding();
}
void
@@ -206,14 +206,16 @@
const Interest& interest2 = *interest;
auto i = m_pendingInterestTable.insert(make_shared<PendingInterest>(
std::move(interest), ref(m_scheduler))).first;
- PendingInterest& entry = **i;
- entry.setDeleter([this, i] { m_pendingInterestTable.erase(i); });
+ // InterestCallback may put Data right away and delete the entry from PendingInterestTable.
+ // shared_ptr is retained to ensure PendingInterest instance is valid throughout the loop.
+ shared_ptr<PendingInterest> entry = *i;
+ entry->setDeleter([this, i] { m_pendingInterestTable.erase(i); });
for (const auto& filter : m_interestFilterTable) {
if (filter->doesMatch(interest2.getName())) {
NDN_LOG_DEBUG(" matches " << filter->getFilter());
+ entry->recordForwarding();
filter->invokeInterestCallback(interest2);
- entry.recordForwarding();
}
}
}
diff --git a/tests/unit-tests/face.t.cpp b/tests/unit-tests/face.t.cpp
index 73c7347..ceee9f7 100644
--- a/tests/unit-tests/face.t.cpp
+++ b/tests/unit-tests/face.t.cpp
@@ -325,6 +325,28 @@
BOOST_CHECK(face.sentData[1].getTag<lp::CongestionMarkTag>() != nullptr);
}
+BOOST_AUTO_TEST_CASE(PutMultipleData)
+{
+ bool hasInterest1 = false;
+ // register two Interest destinations
+ face.setInterestFilter("/", bind([&] {
+ hasInterest1 = true;
+ // sending Data right away from the first destination, don't care whether Interest goes to second destination
+ face.put(*makeData("/A/B"));
+ }));
+ face.setInterestFilter("/", bind([]{}));
+ advanceClocks(time::milliseconds(10));
+
+ face.receive(*makeInterest("/A"));
+ advanceClocks(time::milliseconds(10));
+ BOOST_CHECK(hasInterest1);
+ BOOST_CHECK_EQUAL(face.sentData.size(), 1);
+ BOOST_CHECK_EQUAL(face.sentData.at(0).getName(), "/A/B");
+
+ face.put(*makeData("/A/C"));
+ BOOST_CHECK_EQUAL(face.sentData.size(), 1); // additional Data are ignored
+}
+
BOOST_AUTO_TEST_CASE(PutNack)
{
face.setInterestFilter("/", bind([]{})); // register one Interest destination so that face can accept Nacks
@@ -357,20 +379,27 @@
BOOST_AUTO_TEST_CASE(PutMultipleNack)
{
- face.setInterestFilter("/", bind([]{}));
- face.setInterestFilter("/", bind([]{})); // register two Interest destinations
+ bool hasInterest1 = false, hasInterest2 = false;
+ // register two Interest destinations
+ face.setInterestFilter("/", [&] (const InterestFilter&, const Interest& interest) {
+ hasInterest1 = true;
+ // sending Nack right away from the first destination, Interest should still go to second destination
+ face.put(makeNack(interest, lp::NackReason::CONGESTION));
+ });
+ face.setInterestFilter("/", bind([&] { hasInterest2 = true; }));
advanceClocks(time::milliseconds(10));
face.receive(*makeInterest("/A", 14333271));
advanceClocks(time::milliseconds(10));
+ BOOST_CHECK(hasInterest1);
+ BOOST_CHECK(hasInterest2);
- face.put(makeNack("/A", 14333271, lp::NackReason::CONGESTION)); // Nack from first destination
- advanceClocks(time::milliseconds(10));
- BOOST_CHECK_EQUAL(face.sentNacks.size(), 0); // should wait for Nacks from all destinations
+ // Nack from first destination is received, should wait for a response from the other destination
+ BOOST_CHECK_EQUAL(face.sentNacks.size(), 0);
face.put(makeNack("/A", 14333271, lp::NackReason::NO_ROUTE)); // Nack from second destination
advanceClocks(time::milliseconds(10));
- BOOST_CHECK_EQUAL(face.sentNacks.size(), 1); // both destinations Nacked
+ BOOST_CHECK_EQUAL(face.sentNacks.size(), 1); // sending Nack after both destinations Nacked
BOOST_CHECK_EQUAL(face.sentNacks.at(0).getReason(), lp::NackReason::CONGESTION); // least severe reason
face.put(makeNack("/A", 14333271, lp::NackReason::DUPLICATE));