Heap corruption detected while freeing memory

I have following code in which I am getting "Heap corruption detected" message while trying to freeing memory allocated while using strdup().

Code:

---

#include <iostream>
#include <string>
#include "Check.h"


static void main()
{

       
for (int i = 0;i< 5 ;i++)
{
char cname[32];
cname = GetGroupName()//These function assigns name here
bool exist;
        exist =        GetGroupStatus(cname);
}

}

bool GetGroupStatus(char *cname)
{
        char * cTemp = NULL;
        cTemp = _strdup(cname);
        bool bGrpStatus;
        size_t found;

        //some code here

        std::string sTemp = cTemp;
       
        found = sTemp.find("GROUP");

        if (found != string::npos)
                bGrpStatus = true;
        else
                bGrpStatus = false;

        if (cTemp  != NULL)
        free(cTemp);        //These causes heap corruption detected message when control comes here second time

        return bGrpStatus;
}

---

Please let me know what I am missing?

Hi,

Can you show the content and prototype for GetGroupName()?

Alan

Please put your code between code tags [ c o d e ] ... [ / c o d e ].

Quote:

---

free(cTemp); //These causes heap corruption detected message when control comes here second time

---

I can't see from your code how control could come twice to that point. But if you would set the pointer to NULL after freeing (every time) even recursive callings would be safe.

GetGroupStatus() is called in for loop from Main.So when i = 0 it works fine , after that when I tries to free the memory throws an error message.Would it be good practise to set again to NULL after freeing.

if (cTemp != NULL)
free(cTemp);


*cTemp = NULL

That check for a null pointer is useless because calling free on a null pointer is already safe. The null pointer check that you should be performing is right after the _strdup call. Setting the pointer to be a null pointer after the free is also not very useful because the only line after that is the return statement.

I suspect that the problem lies in the code that you did not show us, e.g., the "//some code here" part. Why do you need to use _strdup when you are able to use std::string?

Code at that place will function something like this -
//first check is added depending on one of the runtime value
//as it is true cTemp will be assigned with new value


I have restriction of using char * on that places,wherever possible I am using std::string.

Quote:

---

Originally Posted by forstudy3

GetGroupStatus() is called in for loop from Main.So when i = 0 it works fine , after that when I tries to free the memory throws an error message.Would it be good practise to set again to NULL after freeing.

if (cTemp != NULL)
free(cTemp);


*cTemp = NULL

---

It should be cTemp = NULL; cause cTemp is the pointer and not *cTemp.

Setting to NULL after free won't solve the problem if the cTemp is a local variable and you would return after that. But it is not useless cause the old pointer value isn't valid after free, hence it is less error-prone for future enhancements to initialize the invalid pointer variable again.

If the cTemp is a shared variable (either member variable or global variable) it is essential that you initialize it to NULL after free.

Looking at the code you posted I would assume that you already freed the pointer somewhere else. You can avoid such mistakes by always freeing pointers on the same calling level where you made the malloc or strdup. So if you passed a pointer to a function never free it in the function called but always in the calling function.

Quote:

---

Originally Posted by forstudy3

Code at that place will function something like this -
//first check is added depending on one of the runtime value
//as it is true cTemp will be assigned with new value

---

Yes, that has the potential to contain a problem: you are messing around with cTemp there, yet you did not show the code. itsmeandnobodyelse's suspicion that "you already freed the pointer somewhere else" may well prove valid, even though it is all on the same calling level.

EDIT:

Quote:

---

Originally Posted by forstudy3

I have restriction of using char * on that places,wherever possible I am using std::string.

---

If feasible, you could use a std::vector<char> instead. You could also use a std::string and then access &cTemp[0] for a char*, but there's a slight risk that until the next version of the C++ standard comes out it might not be correct.

By the way, the global main function should not be declared static.

Quote:

---

Originally Posted by forstudy3

I have restriction of using char * on that places,wherever possible I am using std::string.

---

First, your program has a lot of issues:

Code:

---

static void main()

---

Is this your invention? The main() function is not static, and returns an int, not void.

Second, your GetGroupStatus function does not alter cname in any way. Therefore that parameter should be const char*, not char*.

Once it's const char*, then you don't need to use char* within that function at all.

Code:

---

bool GetGroupStatus(const char *cname)
{
    std::string sTemp = cname;
    bool bGrpStatus;
    size_t found;

    //some code here (what is this?????)

    found = sTemp.find("GROUP");

    if (found != string::npos)
        bGrpStatus = true;
    else
        bGrpStatus = false;

        return bGrpStatus;
}

---

There is no need for strdup() or free().

For practically all cases, there is no need to introduce 'C' functions, char pointers and dynamic memory when handling strings in C++. Show us the case in your code where you must do this -- chances are, you don't need to do anything of the sort.

Regards,

Paul McKenzie