All geek questions in one place

How to fix "Incorrect reading of size 8 - 40 bytes inside a block of size 64 for free"

The Share_ptr in the SPacket in m_PhyToBtMap seems to cause "Invalid reads of 8 to 40 bytes inside a block of size 64 for free." Note: this went on for nearly 22 hours with millions of messages before valgrind (log below) issued this error message, but I also get SIGSEGV crashes in EraseAcknowledgedPackets (see below) and suspect that this is the reason. I am using Boost 1.63 since the cross-compiler does not support shared_ptr. The valgrind log calls SendMessageToBt (invalid read size 8) and EraseAcknowledgedPackets (40 bytes inside a block of 64 free).

  • Am I using shared_ptr or make_shared incorrectly?
  • Do I need to add any checks before I do map.erase or insert?
  • Do I understand valgrind log correctly?

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)
+5
source share
1 answer

I created this SSCCE Live On Coliru to realize any of my assumptions:

 #include <boost/make_shared.hpp> #include <boost/thread.hpp> constexpr unsigned SEQUENCE_NUMBER_MIN = 100u; struct SPacket { boost::shared_ptr<uint8_t[]> data; size_t size; }; struct RadioManager { void SendMessageToBt(uint8_t const* response, size_t responseSize); void SetCrc32(uint8_t * buffer, size_t offset) { buffer[offset++] = 0; // TODO buffer[offset++] = 0; buffer[offset++] = 0; buffer[offset++] = 0; } void EraseAcknowledgedPackets(uint16_t acknowledgementNumber); private: mutable boost::mutex m_LockPhyToBt; using CSrProtected = boost::mutex::scoped_lock; std::map<uint16_t, SPacket> m_PhyToBtMap; uint16_t m_NextReceiveSequenceNumber = SEQUENCE_NUMBER_MIN; uint16_t m_AcknowledgementNumberSent; uint16_t m_NextSendSequenceNumber = SEQUENCE_NUMBER_MIN; uint16_t m_SequenceNumberSent; enum { DATAGRAM_ACKNOWLEDGEMENT_LSB = 0, DATAGRAM_ACKNOWLEDGEMENT_MSB = 1, DATAGRAM_HEADER_SIZE = 8, SEQUENCE_NUMBER_LIST_SIZE_LSB = 0, SEQUENCE_NUMBER_LIST_SIZE_MSB = 1, MESSAGE_HEADER_OFFSET = 10, // CRC32_SIZE = 4 }; }; void RadioManager::EraseAcknowledgedPackets(uint16_t acknowledgementNumber) { CSrProtected ThreadSafe(m_LockPhyToBt); auto 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 } void RadioManager::SendMessageToBt(uint8_t const* 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); auto data = sentPacket.data.get(); assert(data); memcpy(data + MESSAGE_HEADER_OFFSET, response, responseSize); m_AcknowledgementNumberSent = m_NextReceiveSequenceNumber; m_SequenceNumberSent = m_NextSendSequenceNumber; if (0 == ++m_NextSendSequenceNumber) m_NextSendSequenceNumber = SEQUENCE_NUMBER_MIN; m_PhyToBtMap[m_SequenceNumberSent] = sentPacket; // RadioManager.cpp:246 data[DATAGRAM_ACKNOWLEDGEMENT_LSB] = m_AcknowledgementNumberSent; data[DATAGRAM_ACKNOWLEDGEMENT_MSB] = m_AcknowledgementNumberSent >> 8; data[DATAGRAM_HEADER_SIZE + SEQUENCE_NUMBER_LIST_SIZE_LSB] = m_SequenceNumberSent; data[DATAGRAM_HEADER_SIZE + SEQUENCE_NUMBER_LIST_SIZE_MSB] = m_SequenceNumberSent >> 8; SetCrc32(data, sentPacket.size - CRC32_SIZE); //m_Socket->SendTo(data, sentPacket.size); } int main() { RadioManager rm; uint8_t message[] = "HELLO WORLD"; rm.SendMessageToBt(message, sizeof(message)); rm.SendMessageToBt(message, sizeof(message)); rm.EraseAcknowledgedPackets(SEQUENCE_NUMBER_MIN); rm.SendMessageToBt(message, sizeof(message)); rm.SendMessageToBt(message, sizeof(message)); }

Looking at it long and hard, I can only see

  • the above code does not cause problems (it is assumed that CSrProtected really the scope of the "critical section", therefore, like std::lock_guard<std::mutex> ).

  • For this line inside the SendMessageToBt to read, in order to call it, it actually has to be erroneously passed out. In other words, the message says that it is trying to read 8 bytes by assigning data SPacket member.

    It also reports that the address is 40 bytes in the 64-byte block previously allocated by shared-ptr (so something lives inside a char[] allocated to another shared pointer).

    Since we know that sentPacket completely on the stack, “something” cannot be the shared_ptr<> object itself, and since the uint8_t[] data is not copied when copying the shared pointer, we know that it MUST be a control unit.

    However, this makes no sense, because shared_ptr ensures that the control unit does not move or disappear until the last instance of shared_ptr<> disappears. And we have at least 1 on the stack, so ...

All this leads to only one possible result: there is Undefined Behavior in another place (perhaps an instance of SPacket on the stack?). An incredible source of UB is really the lack of thread synchronization.

  • Make sure CSrProtected doing what you think.
  • Make sure that ALL operations on member data are synchronized in the corresponding critical section (s)

Other notes:

  • It's a little strange to have a size section. Now the data lifetime is shared, but the size is not equal. Why not replace SPacket with something like shared_ptr<vector<uint8_t> > so that you
    • don't have this weird split
    • can use vector::at instead of unverified indexing operator[] . This will protect you from external constraints (what if DATAGRAM_HEADER_SIZE + SEQUENCE_NUMBER_LIST_SIZE_LSB turns out to be> MESSAGE_HEADER_OFFSET , etc.).

You are already using Valgrind. Perhaps changing the ASan / UBSan compiler might help.

+4
source

Source: https://habr.com/ru/post/1271857/

More articles:


All Articles