0

We are having a 'Entry' class like below and std map with set of Entries. at the end we clean the map but valgrind shows possible leaks of the memory allocated via strdup function.

class Entry
{
public:
    char* m_key;
    t m_data;

    Entry(const char* key, t data)
    {
        m_key= strdup(key);
        m_data = data;
    }

    ~Entry()
    {
        free((void*)m_key);
        m_key = nullptr;
        delete m_data;
        m_data = nullptr
    }
};

Insert elements to the Map

std::map<const char*, Entry*, Predicate> map_Entries;

void Insert(const char* key, t data)
{
   Entry *pNewEntry = new Entry(key, data);
   map_Entries.insert(std::pair<const char*, Entry*>(pNewEntry->m_key, pNewEntry));
}

Clear element from the map

void Clear()
{
  typename std::map<const char*, Entry*, Predicate>::iterator it;
    for(it = map_Entries.begin(); it != map_Entries.end(); it++)
        delete (*it).second;

    map_Entries.clear();
}

Predicate is comparison operator like below.

struct Mapcasestr
{
  bool operator()(const char* s1, const char* s2) const { return strcasecmp(s1, s2) < 0; }
};

Valgrind issue

==5666==    at 0x4C29BC3: malloc (vg_replace_malloc.c:299)
==5666==    by 0x8098AF9: strdup (in /usr/lib64/libc-2.17.so)
 ==5666==    by 0x4E92B1: Entry::Entry(char const*, T*)

I am aware that using raw pointers as map keys is not a good practice and we have better options than strdup. But this is an old code base and I am trying to fix memory leaks there. What I want to know is why valgrind report possible leaks even though we free memory allocated via strdup function?

ShaL
  • 141
  • 6
  • The memory leak can be anywhere in your code, what makes you believe that the leak occurs specifically in the shown parts of the code? A more interesting question would be whether `Clear()` makes demons fly out of everyone's noses by deleting the strings the map's keys point to, and the map references indirectly, before deleting the entire map. – Sam Varshavchik Jan 12 '21 at 03:15
  • 1
    When you use a pointer as the key in a map, it's the actual *pointer* which is the key, not what it might point to. That makes them very bad as Keys, and makes it harder to read and understand your code. – Some programmer dude Jan 12 '21 at 03:15
  • And generally, for any kind of string needs, use `std::string`. It will make your problem disappear. My recommendation is to always try and follow *the rule of zero*. Also I don't see the need to store pointers in the map data. Why do you do that? – Some programmer dude Jan 12 '21 at 03:20
  • Also a note about using the same pointer as both key and member of the class... It will lead to *undefined behavior* as you free the memory, while the map still have a pointer to that memory. – Some programmer dude Jan 12 '21 at 03:24
  • Finally about the Valgrind message and the shown code. Please try to always include a proper [mcve] in your question, and copy-paste the full and complete error output into the question. – Some programmer dude Jan 12 '21 at 03:26
  • This doesn't address the question, but you don't need the cast in `free((void*)m_key);`. `free(m_key);` is sufficient and doesn't raise red flags. Also, `m_key = nullptr;` and `m_data = nullptr;` in the destructor are pointless; the object is going away, so its fields are no longer accessible. – Pete Becker Jan 12 '21 at 15:13

0 Answers0