Advertisement

free( ) failing?????? HELP!!!!

Started by June 04, 2001 11:35 AM
20 comments, last by KaneBlackflame 23 years, 8 months ago
Your function''s structure is fine. The problem is you''re allocating from:
for i=0; i<10000; i++

and you''re deleting from:
for i=0; i<10001 </b> ; i++

Hence the access violation. Change the 10001 to a 10000. And learn to use the debugger--when you get the access violation in free, you can pop back up the stack and see what the value of ''i'' is when the error occured.

Also, if you want your code to look nice when you post it, include it within:
or
[ source ] [ /source ] (without the spaces)

I''d also choose a frame size larger than 60 bytes, but I don''t know the kind of operations your doing so that''s just speaking out of my hat. Something around 4k is more reasonable.

Here''s the complete source I tested with the change I described:
  #include <stdlib.h>int main (){    char **FileFrame=(char**)malloc(sizeof(char*)*100000);    if(FileFrame!=NULL)		    {		        for(int i=0;i<100000;i++)				        {            FileFrame[i]=(char*)malloc(60);            if(FileFrame[i]==NULL)            {                return(-1);            }        }    }    for(int i=0;i<100000;i++)    {        free(FileFrame[i]);    }    free(FileFrame);    return 0;}  


99% of memory allocation errors come from:
- accessing an uninitialized pointer
- accessing a deleted pointer.
- running past array bounds
Ok, to make a note, I am not stupid. I know how to use a debugger very well and know the values of everything, including i, at the time of the crash. In my previous posts, I mention that the crash occurs at about i=8200, but each time I run the program, it crashes at a different i value around 8200. Next, I allocate 100,001 elements to avoind going off the end by mistake, and I changed the code so my posts would not be confusing, but I forgot to change the delete code...sorry, my bad. So, I really have stumped eveyone then? No one can fix this? No uninitialized pointers are accessed, no deleted pointers are accessed, and no bounds are broken. Why does free fail? Why does it work to a point? Have I actually created a problem unsolvable or yet unseen by everyone? I can''t tell wether to be proud or shoot my foot off...
"Victims...aren't we all?" -Brandon Lee, the Crow
Advertisement
Well, if that''s the case, then this can be considered "works for me", since if you run my program on your computer it should work. Something else is going on that is getting lost in the translation to this board.

Forgive me for implying you''re stupid--I assumed you had done as much when you insinuated in your title that the standard memory allocation routines were at fault, rather than some error of your own making.
Ok, here goes. I have 2 functions that I call when I do this function...the main processing routine and the loading routine. I''m going to post them after this useing previously suggested tags, so here goes....NOTE: (The main processing function accepts a ''~'' delimited string containing keywords to remove from a file)

//this is the main chunk loading functionchar LoadList(char Filename[], char **FileData, char *FileBuffer, long *EntryNum, long *DataSoFar, long *DataTotal, char sort){			static unsigned long PageAvailable;	static unsigned long DataRead;	static unsigned char exit;	static unsigned int k;		static unsigned long TempCount, TempTotal;	ifstream TheList(Filename,ios::binary); 		if(TheList.fail())		return(90);		TheList.seekg(*DataSoFar);		TheList.read(FileBuffer,6000000);			PageAvailable=(TheList.tellg())-(*DataSoFar);	exit=0;	TempCount=0;	DataRead=0;	TempTotal=(*DataTotal);		while(TempCount<100000)	{		k=0;		while(FileBuffer[DataRead]>=33)		{			FileData[TempCount][k]=FileBuffer[DataRead];			DataRead++;			k++;							if(DataRead>=PageAvailable)				break;		}		DataRead+=1;					if(FileData[TempCount][0]>=33)		{					FileData[TempCount][k]=''\n'';			FileData[TempCount][++k]=''\r'';			FileData[TempCount][k+1]=NULL;						FileData[TempCount][59]=(char)k;			TempCount++;		}				if(DataRead>=PageAvailable)			break;	}			TheList.close();		TempCount--;		(*EntryNum)=TempCount;	*DataSoFar+=DataRead;			return(0);}//This is the function called to process the keywordslong APIENTRY StrainList(char File[], char Keyword[], long KeyNum, int KeepThere, char TempFile[], void (__stdcall *pfnVBFoo)(long)){	char *FileBuffer, **FileFrame, **KeyFrame;	long FileSoFar=0,FileTotal,FileCount,ReturnCount=0,i,j,k;	long Count,CountTemp;			ifstream FileData(File,ios::nocreate | ios::binary);		ofstream TempData(TempFile);	if(FileData.fail() || TempData.fail())		return(-2);	FileData.seekg(0,ios::end);	FileTotal=FileData.tellg();			FileSoFar=0;	FileData.close();	FileBuffer=(char*)malloc(6000000);	if(FileBuffer==NULL)	{					return(-1);	}				FileFrame=(char**)malloc(sizeof(char*)*100001);	if(FileFrame!=NULL)			{				for(i=0;i<100001;i++)						{			FileFrame=(char*)malloc(60);			if(FileFrame==NULL)<br>			{<br>				return(-1);<br>			}<br>		}<br>	}<br>	else<br>	{<br>		return(-1);<br>	}<br><br>	KeyFrame=(char**)malloc(sizeof(char*)*KeyNum);<br>	if(KeyFrame!=NULL)		<br>	{		<br>		for(i=0;i<KeyNum;i++)				<br>		{<br>			KeyFrame=(char*)malloc(60);<br>			if(KeyFrame==NULL)<br>			{<br>				return(-1);<br>			}<br>		}<br>	}<br>	else<br>	{<br>		return(-1);<br>	}<br>	<br>	j=0;<br>	for(i=0;i<KeyNum;i++)<br>	{		<br>		k=0;<br>		while(Keyword[j]!=''~'')<br>		{<br>			KeyFrame[k]=Keyword[j];		<br>			k++;<br>			j++;<br>		}<br>		j++;<br>		KeyFrame[k]=NULL;<br>	}<br><br>	Count=CountListNoExport(File);<br>	CountTemp=0;<br><br><br>	while(FileSoFar<FileTotal)<br>	{			<br>		pfnVBFoo((double)((double)CountTemp/(double)Count)*100);<br>		//pfnVBFoo(69);<br>		DoEvents();<br>		LoadList(File,FileFrame,FileBuffer, &FileCount, &FileSoFar, &FileTotal,0);<br><br>		for(i=0;i<FileCount;i++)<br>		{		<br>			k=0;<br>			for(j=0;k==0 && j<KeyNum;j++)<br>			{<br>				strcpy(FileBuffer,FileFrame);<br>				k+=(long)strstr(FileFrame,KeyFrame[j]);<br>				strcpy(FileBuffer,FileFrame);<br>			}<br>			if(k)<br>			{<br>				if(KeepThere)<br>				{<br>					strcpy(FileBuffer,FileFrame);<br>					TempData.write(FileBuffer,FileFrame[59]);<br>					ReturnCount++;<br>				}<br>			}<br>			else<br>			{<br>				if(!KeepThere)<br>				{<br>					strcpy(FileBuffer,FileFrame);<br>					TempData.write(FileBuffer,FileFrame[59]);<br>					ReturnCount++;<br>				}<br>			}<br>		}<br>		CountTemp+=FileCount;<br>	}<br><br>	TempData.close();<br><br><br>	free(FileBuffer);<br>	for(i=0;i<100001;i++)<br>	{<br>		free(FileFrame);			<br>	}<br>	free(FileFrame);<br>	for(i=0;i<KeyNum;i++)<br>	{<br>		free(KeyFrame);			<br>	}<br>	free(KeyFrame);<br>	<br>	pfnVBFoo(100);<br>	DoEvents();<br><br>	return(ReturnCount);<br>}<br><br><br>Could someone please look at the code and find something new we have all missed?  My eyes just don''t catch anything in this mess anymore.  If anyone has a specific question, they can ask me directly on AIM, username: KaneBlackflame    Thanks a lot for everyones help so far!<br>  </i>   </pre> 
"Victims...aren't we all?" -Brandon Lee, the Crow
I just tryed converting the allocation from C to C++ and use new and delete instea, and it crashed in the same place, in a function 4 calls deep called void __cdecl __sbh_free_block (PHEADER pHeader, void * pvAlloc) when it tries to access this section of code :

  void __cdecl __sbh_free_block (PHEADER pHeader, void * pvAlloc){    PREGION         pRegion;    PGROUP          pGroup;    PENTRY          pHead;    PENTRY          pEntry;    PENTRY          pNext;    PENTRY          pPrev;    void *          pHeapDecommit;    int             sizeEntry;    int             sizeNext;    int             sizePrev;    unsigned int    indGroup;    unsigned int    indEntry;    unsigned int    indNext;    unsigned int    indPrev;    unsigned int    offRegion;    //  region is determined by the header    pRegion = pHeader->pRegion;    //  use the region offset to determine the group index    offRegion = (unsigned int)pvAlloc - (unsigned int)pHeader->pHeapData;    indGroup = offRegion / BYTES_PER_GROUP;    pGroup = &pRegion->grpHeadList[indGroup];    //  get size of entry - decrement value since entry is allocated    pEntry = (PENTRY)((char *)pvAlloc - sizeof(int));    sizeEntry = pEntry->sizeFront - 1;    //  point to next entry to get its size    pNext = (PENTRY)((char *)pEntry + sizeEntry);    sizeNext = pNext->sizeFront;/////////////////////////////////////////////fails here/////////////////    //  get size from end of previous entry    sizePrev = ((PENTRYEND)((char *)pEntry - sizeof(int)))->sizeBack;  


Any ideas?
"Victims...aren't we all?" -Brandon Lee, the Crow
Please correct if I''m wrong on any of this,
I don''t know your code very well, we just met and I''m shy
and your code''s bashful(sorry for the jokes but there are more)
(starting with StrainList)
1) first you declare what var''s you need(duh)
2) then 6,000,000 bytes allocated for Filebuffer
3) then 100,000 60 byte strings allocated for FileFrame
then a similar process for Keyframe and then the keyword
being searched for is put into Keyframe from Keyword(go fig)
4) then we hit a while loop, but DoEvents() isn''t listed, hope
it''s not important, pfnVBFoo-have no clue what the hell that
is either,
5) LoadList, well, it loads a list, who knew?, from disk to
FileBuffer to FileData(which is really FileFrame)
6) then a for loop looks through each FileFrame for the Keyword
which is now stored in KeyFrame, and a lot of string
manipulation happens which I hope works the way you want it
to cause TempData.write isn''t listed
(I see how it works but where did you come up with
k+=(long)strstr(FileFrame,KeyFrame[j]);
I thought strstr() returned a pointer and k is a long)
7) then FileBuffer, FileFrame, and Keyframe are set free()

You might try to make FileFrame into an array of pointers
instead of using multiple indirection.
Also, keeping a file buffer so that only 6 megs of memory is
in use at a time was a good idea but you''re really using 12
because everything from FileBuffer is being copied into
FileFrame''s many many 60 byte strings, why not just search
through FileBuffer directly or make FileFrame a temporary
string and search through FileBuffer 60 bytes at a time.
This right here might be a problem:
for(i=0;i<100001;i++)
FileFrame=(char*)malloc(60);
if(FileFrame==NULL) <br>perhaps it should be FileFrame=(char*)malloc(60); </i>
Advertisement
Wow. So many magic numbers. I would suggest you immediately
convert any hard-coded number you see in that code to refer
to static const int, static const unsigned int or #define
at the top of your source file.

You're allocating a massive amount of memory here, but your
problem is on free and you're checking all your allocation
commands, so that shouldn't be a problem.

Your key-scanning routine will be much more user-friendly if you
use strtok. You can pass '~' as a token, and won't need to have
the use specify in two places (i.e. the string of keys and the
key count) how many keys there are. I got some nifty access
violations till I figured out I needed to end the list with a
tilde as well.

You're opening and closing the file each time through LoadList.
Is this a good idea? Of course, you're reading 6 megs at a
time, but in a 600meg file, thats 100 opens & closes for no
reason.

You're using the old ifstream, i.e. the one in "fstream.h". I
can tell because nocreate isn't a standard flag. I'd suggest
using "fstream" and the std::ifstream, since it's actually
standard. It's also a bit weird that you're mixing C++
libraries and C libraries, but that's OK I guess.

You're not checking for fail after TheList.read, nor are you
making sure you're not trying to read past the end of the file.
For the file I used (1 meg), it did indeed fail because reading
more than what's left is a failure. If you're using the
standard fstream, this will cause subsequent calls to fstream to
return odd values, i.e. TheList.tellg () returns -1. This
doesn't seem to happen for the old nonstandard fstream you're
using, though.

In LoadFile, again while reading keys into FileData, strtok
would help a lot. Also, you seem to be inserting an off-by-one
error when you return the value from LoadFile (returns 9999).
But you don't use the return value, so no point.

Back in StrainList, what is the purpose of these lines?:
      strcpy(FileBuffer,FileFrame[i]);k+=(long)strstr(FileFrame[i],KeyFrame[j]);strcpy(FileBuffer,FileFrame[i]);      

The first line copies the current FileFrame into your
FileBuffer. The second line increments k if FileFrame is<br>in KeyFrame[j], but doesn't touch FileBuffer. The third line<br>recopies it. In fact, you copy data into FileBuffer all over<br>the place, but it's never used except by the LoadFile function.<br><br>This function is spaghetti code of the worst kind. It doesn't<br>surprise me that it breaks. I wasn't able to get it to break<br>for my one meg file with a couple of keys, so there must be some <br>combination of the keys you use and the data you have that <br>breaks your code.<br><br>The problem is not with free–it's with your slop. Since the <br>mallocs and frees all look right, there's probably a logical <br>combination of your plethora of inputs that causes some array to <br>overflow its bounds, causing pointers to be overwritten that you <br>later try to delete. To check this, output the values of your <br>pointers when you malloc, then output them as you try to free. <br>I'm willing to bet they don't match.<br><br>Edited by - Stoffel on June 5, 2001 4:45:27 PM<br><br>Edited by - Stoffel on June 5, 2001 4:50:04 PM
How does free() know how much to free?
Maybe try pointer arithmetic?
Why is the algorithm so complex, why not
char FileFrame[100000][60] and do free() only once? or
char *FileFrame[100000]
free(&FileFrame[9998][0])
free(FileFrame[998])
Ok, as far as static arrays, they make your exe''s bulky and fat, so I don''t use them unless they are small. Now to Stoffel...I checked all of the pointers so that each memchunk I free is verified with duplicate record to make sure no changed occured. It still crash even though the addresses matched. If it is trying to free properly allocated memory and failing, what is the issue? Nice try though...keep the idea coming...
"Victims...aren't we all?" -Brandon Lee, the Crow
tried a complete rebuild of all project modules (i.e. if the main exe uses some custom dlls, rebuild them too)?

This topic is closed to new replies.

Advertisement