Memory leak in LoRaLayer2.cpp

beegee-tokyo commented 4 years ago

In int LL2Class::sendToLayer1(struct Packet packet) and struct Packet LL2Class::buildPacket( uint8_t ttl, uint8_t src[6], uint8_t dest[6], uint8_t sequence, uint8_t type, uint8_t data[240], uint8_t dataLength) you use malloc to allocate memory from the heap, but the memory is not freed up when leaving the functions.

int LL2Class::sendToLayer1(struct Packet packet) {

    uint8_t* sending = (uint8_t*) malloc(sizeof(packet));
    memcpy(sending, &packet, sizeof(packet));
    /*
    int send = 1;
    if(hashingEnabled){
        // do not send message if already transmitted once
        //uint8_t hash[SHA1_LENGTH];
        //SHA1(sending, packet.totalLength, hash);
        //if(isHashNew(hash)){
        //  send = 0;
        //}
    }
    */
    Layer1.send_packet((char*) sending, packet.totalLength);
    _messageCount++;
    return _messageCount;
}

should be

int LL2Class::sendToLayer1(struct Packet packet) {

    uint8_t* sending = (uint8_t*) malloc(sizeof(packet));
    memcpy(sending, &packet, sizeof(packet));
    /*
    int send = 1;
    if(hashingEnabled){
        // do not send message if already transmitted once
        //uint8_t hash[SHA1_LENGTH];
        //SHA1(sending, packet.totalLength, hash);
        //if(isHashNew(hash)){
        //  send = 0;
        //}
    }
    */
    Layer1.send_packet((char*) sending, packet.totalLength);
    _messageCount++;
    free(sending);
    return _messageCount;
}

and similar in the other function. I stumbled over this with frequent reboots while implementing the BLE client which uses up much more memory than WiFi. The exception decoder always points to the same line in the code. Maybe a check if the malloc was successful should be added as well.

beegee-tokyo commented 4 years ago

Found something weird in struct Packet LL2Class::buildPacket() You allocate memory and put the pointer to it into uint8_t buffer. But in the next line you alter the pointer to point to data.

uint8_t* buffer = (uint8_t*)  malloc(dataLength);
buffer = (uint8_t *)data;

When I tried to free(buffer) the code crashed of course. As you're not using the allocated memory at all, the solution would be simply

struct Packet LL2Class::buildPacket( uint8_t ttl, uint8_t src[6], uint8_t dest[6], uint8_t sequence, uint8_t type, uint8_t data[240], uint8_t dataLength){

    uint8_t packetLength = HEADER_LENGTH + dataLength;
    // uint8_t* buffer = (uint8_t*)  malloc(dataLength);
    // buffer = (uint8_t *)data;
    struct Packet packet = {
        ttl,
        packetLength,
        src[0], src[1], src[2], src[3], src[4], src[5],
        dest[0], dest[1], dest[2], dest[3], dest[4], dest[5],
        sequence,
        type 
    };
    memcpy(&packet.data, data, packet.totalLength);
    // free(buffer);
    return packet;
}

And similar in int LL2Class::sendToLayer1(struct Packet packet). Why not skip the malloc and do simply

int LL2Class::sendToLayer1(struct Packet packet)
{

    // uint8_t* sending = (uint8_t*) malloc(sizeof(packet));
    // memcpy(sending, &packet, sizeof(packet));
    /*
    int send = 1;
    if(hashingEnabled){
        // do not send message if already transmitted once
        //uint8_t hash[SHA1_LENGTH];
        //SHA1(sending, packet.totalLength, hash);
        //if(isHashNew(hash)){
        //  send = 0;
        //}
    }
    */
    Layer1.send_packet((char *)&packet, packet.totalLength);
    _messageCount++;
    // free(sending);
    return _messageCount;
}

paidforby commented 4 years ago

Thanks for pointing these out. Probably stuff that was left in from earlier debugging steps that I removed. I'm in the process of cleaning up and refactoring some of this code.

beegee-tokyo commented 4 years ago

It came up while testing the BLE Client because heap is getting low when using BLE. Still hoping Espressif is improving the BLE heap usage.

sudomesh / LoRaLayer2

Memory leak in LoRaLayer2.cpp #5