How to fix "Invalid read of size 8 - 40 bytes inside a block of size 64 free'd"

Question

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)

It looks like you have two threads both interacting with `m_PhyToBtMap`, without any synchronization. That's just asking for problems. — Miles Budnek, Sep 15 '17 at 22:34
It seems there are two threads (????) one is freeing the memory once a packet has been acknowledged and the other one is sending the acknowledgement. However it looks like they are not correctly synchronized: it tries to delete the packet before the acknowledgement has been sent. From the uploaded code its not possible to tell, but is it possible that the package associated with an acknowledgement number (that is passed in the function EraseAcknowledgementPackets) has not been sent? You would need two counters: one for next acknowledgement number and one for the packets that have been sent. — steve, Sep 15 '17 at 22:37
@steve shared pointers and critical sections. They're both used. — sehe, Sep 15 '17 at 23:15
Here's the same code cleaned up and self-contained http://coliru.stacked-crooked.com/a/1f5168ce58a8fbb4 — sehe, Sep 15 '17 at 23:27
Note that even if your compiler had `std::shared_ptr`, there's no `shared_ptr` in the standard library. It's a boost extension. — sehe, Sep 15 '17 at 23:57

score 4 · Accepted Answer · answered Sep 15 '17 at 23:55

I created this SSCCE Live On Coliru to materialize any assumptions I made:

#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 code shown has no problem (assuming that CSrProtected is indeed a scoped lock of a "critical section", so like std::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 the data member of SPacket.

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 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 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 last shared_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.

Check that CSrProtected does what you think it does
Check that ALL operations on member data synchronize on the proper critical section(s)

Other notes:

It's a bit weird to have the size separate. Now, the lifetime of the data is shared, but the size is not. Why not replace SPacket with something like shared_ptr<vector<uint8_t> > so you
- don't have that weird split
- can use vector::at instead of unchecked operator[] indexing. This will protect you from out-of-bounds (what if DATAGRAM_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.

I haven't proved it yet but it may be that the issue is in CSrProtected. I originally disregarded this possibility since this class is already used in a number of other systems, but my system seems to be happy after replacing CSrProtected with boost::lock_guard. Amazing answer. Thank you very much. — jacknad, Sep 24 '17 at 23:57

How to fix "Invalid read of size 8 - 40 bytes inside a block of size 64 free'd"

1 Answers1