Raizo62 / vwifi

Simulator of WiFi (802.11) interfaces to communicate between several Virtual Machines
GNU Lesser General Public License v3.0
57 stars 8 forks source link

Heap Memory leak in the code #3

Closed subjectxbj closed 3 years ago

subjectxbj commented 3 years ago

First thanks for developing this tool, which is quite helpful.

We found a memory leak issue during test.

void CKernelWifi::recv_from_server(){
        ...
    std::vector<WirelessDevice> inets = _list_winterfaces.list_devices();

    for (auto & inet : inets)
    {
        struct ether_addr macdsthwsim = inet.getMachwsim();

        send_cloned_frame_msg(&macdsthwsim, data, data_len, rate_idx, signal, freq);
    }
}

function "list_devices()" is defined as following

std::vector<WirelessDevice> & WirelessDeviceList::list_devices()  {

    std::vector<WirelessDevice> * list_wd = new std::vector<WirelessDevice>();

    _listaccess.lock();
    for (auto & wd : _wdevices_list){

        list_wd->push_back(wd.second);
    }
    _listaccess.unlock();

    return *(list_wd) ;

}

The return value of "list_devices()" is declared to be a reference variable while "inets" is not. In the case, inets will construct a new object which has different address. and the object pointed by list_wd gets no chance to be freed, which leads to heap memory leak.

Propose fix as following:

void CKernelWifi::recv_from_server(){
        ...
    std::vector<WirelessDevice> &inets = _list_winterfaces.list_devices();

    for (auto & inet : inets)
    {
        struct ether_addr macdsthwsim = inet.getMachwsim();

        send_cloned_frame_msg(&macdsthwsim, data, data_len, rate_idx, signal, freq);
    }
        delete &inets;
}

Or change "list_devices()" to return the pointer instead.

Raizo62 commented 3 years ago

Hi

I applied your patch on the branch "beta".

Thank you very much.

Did you use a tool to find it ?

subjectxbj commented 3 years ago

@Raizo62 I used "valgrind" to find it out.

Raizo62 commented 3 years ago

@subjectxbj , thank you