Actions
Bug #3324
closed
lp::Packet::wireEncode() has nonsensical return type
Start date:
Due date:
% Done:
100%
Estimated time:
Description
Returning a const Block doesn't make sense and prevents move semantics. It should be either Block or const Block&.
Indeed the return type was originally const Block&, but it was changed in commit 83872fd229b8d87e8e2aac25ba1dac3a69740bfd for reasons unknown to me.
http://stackoverflow.com/questions/8716330/purpose-of-returning-by-const-value
Updated by Eric Newberry over 8 years ago
- Status changed from New to In Progress
After looking at this again, I agree that it makes no sense. I believe that removing the const works better. I tried making this change and all of the test cases passed. When changing it to const Block&, the test cases encountered a memory access violation.
Updated by Eric Newberry over 8 years ago
Davide Pesavento wrote:
What test cases?
The ndn-cxx link protocol test cases.
Updated by Eric Newberry over 8 years ago
- Status changed from In Progress to Code review
Updated by Davide Pesavento over 8 years ago
Eric Newberry wrote:
When changing it to
const Block&, the test cases encountered a memory access violation.
It crashes with const Block& because Packet::wireEncode returns a reference to an object that is about to be destructed:
Block::element_container elements = m_wire.elements();
if (elements.size() == 1 && elements.front().type() == FragmentField::TlvType::value) {
elements.front().parse();
elements.front().elements().front().parse();
return elements.front().elements().front();
}
For the record, this is what valgrind says about LpPacket/DecodeBareNetworkLayerPacket:
==4802== Invalid read of size 8
==4802== at 0x47DBEE: operator= (shared_ptr_base.h:867)
==4802== by 0x47DBEE: operator= (shared_ptr.h:93)
==4802== by 0x47DBEE: ndn::Block::operator=(ndn::Block const&) (block.hpp:43)
==4802== by 0x51C2CC: ndn::lp::tests::LpPacket::DecodeBareNetworkLayerPacket::test_method() (packet.t.cpp:323)
==4802== by 0x51C784: ndn::lp::tests::LpPacket::DecodeBareNetworkLayerPacket_invoker() (packet.t.cpp:307)
==4802== by 0x477192: invoke<void (*)()> (callback.hpp:56)
==4802== by 0x477192: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
==4802== by 0x5BA5CD0: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B858B5: boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B860D2: boost::execution_monitor::execute(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5BA5E01: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::unit_test::test_case const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B8CFBD: boost::unit_test::framework_impl::visit(boost::unit_test::test_case const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5BC348A: boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5BC348A: boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B88915: boost::unit_test::framework::run(unsigned long, bool) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== Address 0xa3654f0 is 0 bytes inside a block of size 88 free'd
==4802== at 0x4C2D28B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4802== by 0x500A21D: deallocate (new_allocator.h:110)
==4802== by 0x500A21D: deallocate (alloc_traits.h:386)
==4802== by 0x500A21D: _M_deallocate (stl_vector.h:178)
==4802== by 0x500A21D: ~_Vector_base (stl_vector.h:160)
==4802== by 0x500A21D: ~vector (stl_vector.h:425)
==4802== by 0x500A21D: ~Block (block.hpp:43)
==4802== by 0x500A21D: _Destroy<ndn::Block> (stl_construct.h:93)
==4802== by 0x500A21D: __destroy<ndn::Block*> (stl_construct.h:103)
==4802== by 0x500A21D: _Destroy<ndn::Block*> (stl_construct.h:126)
==4802== by 0x500A21D: _Destroy<ndn::Block*, ndn::Block> (stl_construct.h:151)
==4802== by 0x500A21D: ~vector (stl_vector.h:424)
==4802== by 0x500A21D: ndn::lp::Packet::wireEncode() const (packet.cpp:74)
==4802== by 0x51C2C0: ndn::lp::tests::LpPacket::DecodeBareNetworkLayerPacket::test_method() (packet.t.cpp:323)
==4802== by 0x51C784: ndn::lp::tests::LpPacket::DecodeBareNetworkLayerPacket_invoker() (packet.t.cpp:307)
==4802== by 0x477192: invoke<void (*)()> (callback.hpp:56)
==4802== by 0x477192: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
==4802== by 0x5BA5CD0: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B858B5: boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B860D2: boost::execution_monitor::execute(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5BA5E01: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::unit_test::test_case const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B8CFBD: boost::unit_test::framework_impl::visit(boost::unit_test::test_case const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5BC348A: boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5BC348A: boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== Block was alloc'd at
==4802== at 0x4C2C12F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4802== by 0x4FC8D4C: allocate (new_allocator.h:104)
==4802== by 0x4FC8D4C: allocate (alloc_traits.h:360)
==4802== by 0x4FC8D4C: _M_allocate (stl_vector.h:170)
==4802== by 0x4FC8D4C: void std::vector<ndn::Block, std::allocator<ndn::Block> >::_M_emplace_back_aux<ndn::Block>(ndn::Block&&) (vector.tcc:412)
==4802== by 0x4FC9114: void std::vector<ndn::Block, std::allocator<ndn::Block> >::emplace_back<ndn::Block>(ndn::Block&&) (vector.tcc:101)
==4802== by 0x4FC2DA0: push_back (stl_vector.h:932)
==4802== by 0x4FC2DA0: ndn::Block::parse() const (block.cpp:347)
==4802== by 0x50095EF: ndn::lp::Packet::wireEncode() const (packet.cpp:76)
==4802== by 0x51C2C0: ndn::lp::tests::LpPacket::DecodeBareNetworkLayerPacket::test_method() (packet.t.cpp:323)
==4802== by 0x51C784: ndn::lp::tests::LpPacket::DecodeBareNetworkLayerPacket_invoker() (packet.t.cpp:307)
==4802== by 0x477192: invoke<void (*)()> (callback.hpp:56)
==4802== by 0x477192: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
==4802== by 0x5BA5CD0: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B858B5: boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5B860D2: boost::execution_monitor::execute(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==4802== by 0x5BA5E01: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::unit_test::test_case const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
[several other similar errors follow]
Updated by Junxiao Shi over 8 years ago
- Status changed from Code review to Closed
- % Done changed from 0 to 100
Actions