-
January 10th, 2011, 02:51 AM
#1
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?
Last edited by cilu; January 10th, 2011 at 04:17 AM.
Reason: code tags
-
January 10th, 2011, 04:07 AM
#2
Re: Heap corruption detected while freeing memory
Hi,
Can you show the content and prototype for GetGroupName()?
Alan
-
January 10th, 2011, 04:08 AM
#3
Re: Heap corruption detected while freeing memory
Please put your code between code tags [ c o d e ] ... [ / c o d e ].
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.
-
January 10th, 2011, 04:47 AM
#4
Re: Heap corruption detected while freeing memory
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
-
January 10th, 2011, 04:53 AM
#5
Re: Heap corruption detected while freeing memory
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?
-
January 10th, 2011, 05:00 AM
#6
Re: Heap corruption detected while freeing memory
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.
-
January 10th, 2011, 05:12 AM
#7
Re: Heap corruption detected while freeing memory
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.
-
January 10th, 2011, 05:19 AM
#8
Re: Heap corruption detected while freeing memory
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:
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.
Last edited by laserlight; January 10th, 2011 at 05:26 AM.
-
January 10th, 2011, 05:28 AM
#9
Re: Heap corruption detected while freeing memory
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:
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
Posting Permissions
- You may not post new threads
- You may not post replies
- You may not post attachments
- You may not edit your posts
-
Forum Rules
|
Click Here to Expand Forum to Full Width
|