The shared_ptr in SPacket in m_PhyToBtMap seems to cause "Invalid read of size 8 - 40 bytes inside a block of size 64 free'd." Note: this ran for almost 22 hours with millions of messages before valgrind (log below) issued this error message but I'm also getting SIGSEGV crashes in EraseAcknowledgedPackets (below) and suspect this is the cause. I am using Boost 1.63 since the cross compiler doesn't support shared_ptr. SendMessageToBt (Invalid read of size 8) and EraseAcknowledgedPackets (40 bytes inside a block of size 64 free'd) are called out in the valgrind log.
- I am using shared_ptr or make_shared improperly?
- Do I need to add any checks before I do the map.erase or insert?
- Do I properly understand the valgrind log?
SPacket and m_PhyToBtMap
typedef struct SPacket
{
boost::shared_ptr<uint8_t[]> data;
size_t size;
} SPacket;
map<uint16_t, SPacket> m_PhyToBtMap;
SendMessageToBt
void RadioManager::SendMessageToBt(uint8_t * response, size_t responseSize)
{
CSrProtected ThreadSafe(m_LockPhyToBt);
SPacket sentPacket;
sentPacket.size = MESSAGE_HEADER_OFFSET + responseSize + CRC32_SIZE;
sentPacket.data = boost::make_shared<uint8_t[]>(sentPacket.size);
assert(sentPacket.data.get());
memcpy(&sentPacket.data.get()[MESSAGE_HEADER_OFFSET], response, responseSize);
m_AcknowledgementNumberSent = m_NextReceiveSequenceNumber;
m_SequenceNumberSent = m_NextSendSequenceNumber;
++m_NextSendSequenceNumber;
if (0 == m_NextSendSequenceNumber) m_NextSendSequenceNumber = SEQUENCE_NUMBER_MIN;
m_PhyToBtMap[m_SequenceNumberSent] = sentPacket; // RadioManager.cpp:246
sentPacket.data.get()[DATAGRAM_ACKNOWLEDGEMENT_LSB] = m_AcknowledgementNumberSent;
sentPacket.data.get()[DATAGRAM_ACKNOWLEDGEMENT_MSB] = m_AcknowledgementNumberSent >> 8;
sentPacket.data.get()[DATAGRAM_HEADER_SIZE + SEQUENCE_NUMBER_LIST_SIZE_LSB] = m_SequenceNumberSent;
sentPacket.data.get()[DATAGRAM_HEADER_SIZE + SEQUENCE_NUMBER_LIST_SIZE_MSB] = m_SequenceNumberSent >> 8;
SetCrc32(sentPacket.data.get(), sentPacket.size - CRC32_SIZE);
m_Socket->SendTo(sentPacket.data.get(), sentPacket.size);
}
EraseAcknowledgedPackets
void RadioManager::EraseAcknowledgedPackets(uint16_t acknowledgementNumber)
{
CSrProtected ThreadSafe(m_LockPhyToBt);
if (!m_PhyToBtMap.empty())
{
map<uint16_t, SPacket>::iterator it = m_PhyToBtMap.upper_bound(acknowledgementNumber);
int begin = m_PhyToBtMap.begin()->first;
if (begin > acknowledgementNumber) m_PhyToBtMap.erase(it, m_PhyToBtMap.end()); // acknowledgementNumber rollover
else m_PhyToBtMap.erase(m_PhyToBtMap.begin(), it); // RadioManager.cpp:1113
}
}
valgrind log
Invalid read of size 8
at 0x474F84: void std::swap<unsigned char*>(unsigned char*&, unsigned char*&) (move.h:176)
by 0x47430A: boost::shared_ptr<unsigned char []>::swap(boost::shared_ptr<unsigned char []>&) (shared_ptr.hpp:743)
by 0x473042: boost::shared_ptr<unsigned char []>::operator=(boost::shared_ptr<unsigned char []> const&) (shared_ptr.hpp:526)
by 0x4729B8: SPacket::operator=(SPacket const&) (RadioManager.h:32)
by 0x467C8E: RadioManager::SendMessageToBt(unsigned char*, unsigned long) (RadioManager.cpp:246)
by 0x46B64C: RadioManager::TransmitFecBlockResponseEvent(unsigned char*, unsigned long) (RadioManager.cpp:683)
by 0x4A5FDD: BtToRadioInterfaceClient::ProcessMessage(unsigned char*, unsigned long) (BtToRadioInterface.cpp:174)
by 0x4AB6AD: SrZmqPubSubInterface::ReadThread(void*) (SrZmqPubSubInterface.cpp:220)
by 0x4AC258: SingleParamObjCallback<SrZmqPubSubInterface, void*>::operator()(void*) (CallbackFunctors.h:161)
by 0x4AC1D4: CSrClassThread<SrZmqPubSubInterface, void*>::ThreadProcedure(void*) (SrClassThread.h:21)
by 0x46360A: SrThreadProcedure(void*) (SrThread.cpp:41)
by 0x50BCDC4: start_thread (pthread_create.c:308)
by 0x610273C: clone (clone.S:113)
Address 0xb93c638 is 40 bytes inside a block of size 64 free'd
at 0x4C29131: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x476841: __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<unsigned short const, SPacket> > >::deallocate(std::_Rb_tree_node<std::pair<unsigned short const, SPacket> >*, unsigned long) (new_allocator.h:110)
by 0x47562D: std::_Rb_tree<unsigned short, std::pair<unsigned short const, SPacket>, std::_Select1st<std::pair<unsigned short const, SPacket> >, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, SPacket> > >::_M_put_node(std::_Rb_tree_node<std::pair<unsigned short const, SPacket> >*) (stl_tree.h:374)
by 0x4748BC: std::_Rb_tree<unsigned short, std::pair<unsigned short const, SPacket>, std::_Select1st<std::pair<unsigned short const, SPacket> >, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, SPacket> > >::_M_destroy_node(std::_Rb_tree_node<std::pair<unsigned short const, SPacket> >*) (stl_tree.h:396)
by 0x473AB8: std::_Rb_tree<unsigned short, std::pair<unsigned short const, SPacket>, std::_Select1st<std::pair<unsigned short const, SPacket> >, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, SPacket> > >::_M_erase(std::_Rb_tree_node<std::pair<unsigned short const, SPacket> >*) (stl_tree.h:1127)
by 0x473C8C: std::_Rb_tree<unsigned short, std::pair<unsigned short const, SPacket>, std::_Select1st<std::pair<unsigned short const, SPacket> >, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, SPacket> > >::clear() (stl_tree.h:860)
by 0x4754F1: std::_Rb_tree<unsigned short, std::pair<unsigned short const, SPacket>, std::_Select1st<std::pair<unsigned short const, SPacket> >, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, SPacket> > >::_M_erase_aux(std::_Rb_tree_const_iterator<std::pair<unsigned short const, SPacket> >, std::_Rb_tree_const_iterator<std::pair<unsigned short const, SPacket> >) (stl_tree.h:1757)
by 0x474706: std::_Rb_tree<unsigned short, std::pair<unsigned short const, SPacket>, std::_Select1st<std::pair<unsigned short const, SPacket> >, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, SPacket> > >::erase(std::_Rb_tree_iterator<std::pair<unsigned short const, SPacket> >, std::_Rb_tree_iterator<std::pair<unsigned short const, SPacket> >) (stl_tree.h:848)
by 0x47345E: std::map<unsigned short, SPacket, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, SPacket> > >::erase(std::_Rb_tree_iterator<std::pair<unsigned short const, SPacket> >, std::_Rb_tree_iterator<std::pair<unsigned short const, SPacket> >) (stl_map.h:763)
by 0x46E7A3: RadioManager::EraseAcknowledgedPackets(unsigned short) (RadioManager.cpp:1113)
by 0x46D785: RadioManager::Decode(unsigned char*, unsigned long) (RadioManager.cpp:935)
by 0x4655DE: MsgDispatcher::Decode(UdpState*) (MsgDispatcher.cpp:20)
by 0x465976: SingleParamObjCallback<MsgDispatcher, UdpState*>::operator()(UdpState*) (CallbackFunctors.h:161)
by 0x464CBC: IsimUdpSocket::ReceiveHandler(void*) (IsimUdpSocket.cpp:41)
by 0x465410: SingleParamObjCallback<IsimUdpSocket, void*>::operator()(void*) (CallbackFunctors.h:161)
by 0x46538C: CSrClassThread<IsimUdpSocket, void*>::ThreadProcedure(void*) (SrClassThread.h:21)
by 0x46360A: SrThreadProcedure(void*) (SrThread.cpp:41)
I created this SSCCE Live On Coliru to materialize any assumptions I made:
Looking at it long and hard, I can only see
the code shown has no problem (assuming that
CSrProtected
is indeed a scoped lock of a "critical section", so likestd::lock_guard<std::mutex>
).For that line inside
SendMessageToBt
to read to trigger, it should actually be misreported out-of-bounds. In other words, the message says it's trying to read 8 bytes while assigning thedata
member ofSPacket
.It also reports that the address is 40 bytes into a 64-byte block previously allocated by shared-ptr (so something lives is inside the
char[]
allocated for another shared-pointer).Since we know
sentPacket
is wholly on the stack, the "something" cannot be theshared_ptr<>
object itself, and since theuint8_t[]
data is not copied when copying the shared pointer, we know that it MUST be the control block.However that doesn't make any sense, because
shared_ptr
guarantees that the control block doesn't move or disappear until the lastshared_ptr<>
instance goes away. And we have at least 1 on the stack, so...All this leads up to only 1 possible outcome: there is Undefined Behaviour elsewhere (potentially trashing the
SPacket
instance on the stack?). A not-unlikely source of UB is indeed a possible lack of thread synchronization.CSrProtected
does what you think it doesOther notes:
size
separate. Now, the lifetime of thedata
is shared, but the size is not. Why not replaceSPacket
with something likeshared_ptr<vector<uint8_t> >
so youvector::at
instead of uncheckedoperator[]
indexing. This will protect you from out-of-bounds (what ifDATAGRAM_HEADER_SIZE + SEQUENCE_NUMBER_LIST_SIZE_LSB
happens to be >MESSAGE_HEADER_OFFSET
etc.).You're already using Valgrind. Perhaps the ASan/UBSan compiler switches can be of help.