Allegro.cc - Online Community

Allegro.cc Forums » Programming Questions » Memory leaks according to _CrtDumpMemoryLeaks

Credits go to GullRaDriel for helping out!
This thread is locked; no one can reply to it. rss feed Print
Memory leaks according to _CrtDumpMemoryLeaks
TeaRDoWN
Member #8,518
April 2007
avatar

I get memory leak detection with _CrtDumpMemoryLeaks() even after using free() in the char*. What am I doing wrong or is the dump just not reading the memory correct?

#SelectExpand
1char* mLog.text; 2 3main() 4{ 5char *text = (char*)malloc(sizeof(text)*64); 6sprintf(text, "asdf"); 7log(text); 8free(mLog.text); 9_CrtDumpMemoryLeaks(); 10} 11 12 13log(char* text) 14{ 15mLog.text = text; 16}

I get a memory dump similar to this:

Detected memory leaks!
Dumping objects ->
{544042} normal block at 0x122FB3E0, 256 bytes long.
 Data: <                > CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD
Object dump complete.

Another related question, does anyone know how to put a breakpoint on a memory allaction number? I tried to follow the steps on MSDN (http://msdn.microsoft.com/en-us/library/w2fhc9a3.aspx) but I don't know if I'm doing it wrong or whatever. Using Visual C++ 2008 Express edition.

Sol Blast
Member #9,655
April 2008
avatar

It looks like the 'text' you're dynamically allocating is a local. You're then freeing the unused global variable 'mLog.text' which you've not yet allocated anything too, while the local hangs on to the memory.

Maybe you meant your code to look like this? (i'm not familiar with malloc though).

char* mLog.text;  

main()  
{  
mLog.text = (char*)malloc(sizeof(char)*64);  
sprintf(mLog.text, "asdf");  
log(mLog.text);  
...  
free(mLog.text);  
_CrtDumpMemoryLeaks();  
}

Maybe i'm wrong. Like i said, i'm not familiar with C.

Ron Novy
Member #6,982
March 2006
avatar

What does the '...' mean? Post code.

----
Oh... Bieber! I thought everyone was chanting Beaver... Now it doesn't make any sense at all. :-/

TeaRDoWN
Member #8,518
April 2007
avatar

Sol Blast: If everything was public it could be done like that. But I want log() to be the only function that modifies the content of the mLog.text value.

Ron Novy: Actually the code is much more complex but it cut it out to try to make it easier to gasp. Therefor the "..." in the middle of the main - but you could actually ignore this. Thought that 17 lines of code that causes the same error as 100 lines were easier to discuss. :) Sorry for the confusion...

Sol Blast
Member #9,655
April 2008
avatar

TeaRDoWN said:

But I want log() to be the only function that modifies the content of the mLog.text value.

Well in that case i'd need to see what 'log()' does to be sure, but if it copies the string to mLog.text then your call to free should use the local. Not the global.

free(text);

TeaRDoWN
Member #8,518
April 2007
avatar

Sol Blast: But if I do that then the mLog.text will be cleared too and I want to store the data in that variable.

Ok here is basically the entire code, at least more detailed.

#SelectExpand
1// game.c content 2LOG { 3 char* text[50]; 4}; 5 6LOG mLog; 7 8void log(char* text) 9{ 10 for(int l = 49; l > 0; l--) 11 { 12 mLog.text[l] = mLog.text[l-1]; 13 } 14 mLog.text = text; 15} 16 17void clearLog() 18{ 19 for (int l = 0; l < 50; l++) 20 free(mLog.text[l]); 21} 22 23void main() 24{ 25 ... 26 SND_playSample(snd_id) 27 ... 28 clearLog(); 29} 30END_OF_MAIN() 31 32 33 34 35// sound.c content (no able to access the mLog struct, only log()) 36void SND_playSample(int id) 37{ 38 if (mSnd[id] == NULL) 39 { 40 char *text = (char*)malloc(sizeof(text)*64); 41 sprintf(text, "An error has occured trying to play sound #%i", id); 42 log(text); 43 } 44 else 45 playSample(mSnd[id]); 46 } 47}

BAF
Member #2,981
December 2002
avatar

Ah, the masochistic pleasures of coding in C.

Sol Blast
Member #9,655
April 2008
avatar

TeaRDoWN said:

But if I do that then the mLog.text will be cleared too and I want to store the data in that variable.

Ah i see. Well if you really want to do it that way, then you'd probably be better having mLog responsible for freeing its own memory, rather than doing it yourself. Personally though i'd copy the memory like so...

#SelectExpand
1char* mLog.text; 2 3void log( char* text ) 4{ 5 for(int l = 49; l > 0; l--) 6 { 7 mLog.text[l] = mLog.text[l-1]; 8 } //I don't really understand why you do this... 9 10 sprintf(mLog.text, text ); //Instead of referencing it. Copy it. This gives the source of 'str' responsibility for freeing the memory instead of mLog itself. 11} 12 13main() 14{ 15char* text = (char*)malloc(sizeof(char)*64); //New memory is allocated. 16sprintf(text, "asdf"); 17log(text); //Duplicate it into mLog.text. 18... 19free(text); //Since the string memory has been duplicated, we no longer need to hold on to the local. So free it. mLog.text will not be affected. 20_CrtDumpMemoryLeaks(); 21}

I think that'd solve the problem...

TeaRDoWN
Member #8,518
April 2007
avatar

BAF: Thanks I'll try that. :P

Sol Blast: Actually, I tried that before but got the same result. That's when I got really confused and changed it back since the code for this is less complex.
PS: The loop in the log() is pushing older logged messages and having the newest one in first slot ([0]).

LennyLen
Member #5,313
December 2004
avatar

BAF said:

Ah, the masochistic pleasures of coding in C.

This ain't C:

LOG {
    char* text[50];
};

edit:

Sol Blast said:

sprintf(mLog.text, text );

That should be strncpy(mLog.text, text, sizeof(text));

edit2: forgot the third parameter of strncpy.

Sol Blast
Member #9,655
April 2008
avatar

I'll bear that in mind. Thanks LennyLen ;).

GullRaDriel
Member #3,861
September 2003
avatar

Nonconstructive comment #2635267 for BAF.

TeaRDoWN, when you copy your pointer mLog.text to mLog.text - 1, you don't move the data, just the pointer. Don't checking for the data who'll be out of range is a mistake.

I am making you a C version which (I hope) will be without memory leak. I'll test it with Valgrind and dbx.

"Code is like shit - it only smells if it is not yours"
Allegro Wiki, full of examples and articles !!

TeaRDoWN
Member #8,518
April 2007
avatar

Ok, changed this:

void log(char* text)
{
  ...
  //mLog.text = text;

  mLog.text[0] = (char*)malloc(sizeof(mLog.text[0])*64);
  strcpy(mLog.text[0], text);
  free(text);
}

Still get the same memory leak reports:
{544044} normal block at 0x120FB020, 256 bytes long.
Data: < > CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD

Did anyone know how to breakpoint a memory allocation? It should be possible to use the allocation id (544044) and break at the memory leak IF you execute the program exactly the same way again.

---

GullRaDriel: I know about the pointers thing. I added a check in the for-loop to skip NULL points, made no difference. Here this the actual code without simplifications.

#SelectExpand
1void log(char *text, int color) 2{ 3 if (mLog.text[LOGS-1] != NULL) 4 free(mLog.text[LOGS-1]); 5 6 for(int l = LOGS-1; l > 0; l--) 7 { 8 if (mLog.text[l-1] != NULL) 9 { 10 mLog.text[l] = mLog.text[l-1]; 11 mLog.color[l] = mLog.color[l-1]; 12 } 13 } 14 mLog.text[0] = (char*)malloc(sizeof(mLog.text[0])*64); 15 strcpy(mLog.text[0], text); 16 free(text); 17}

LennyLen
Member #5,313
December 2004
avatar

Are you going to be logging to a file? If so, here's the logging code I use, which is leak free.

log.h:

#ifndef LOG_H
#define LOG_H


void begin_logging(char *filename);
void end_logging();
void add_to_log(char *info);


#endif

log.c:

#SelectExpand
1#include <time.h> 2#include <string.h> 3#include <stdio.h> 4#include <stdlib.h> 5#include "log.h" 6 7 8FILE *log_file; 9char temp[64], log_time[64]; 10time_t now; 11 12 13void begin_logging(char *filename) { 14 15 log_file = fopen(filename, "w"); 16 if (log_file == NULL) { 17 18 printf("Error creating log file: %s.\n", filename); 19 exit(1); 20 } 21 22 add_to_log("Logging begun.\n"); 23 24} 25 26 27void end_logging() { 28 29 add_to_log("Logging ends.\n"); 30 fclose(log_file); 31 32} 33 34 35void add_to_log(char *info) { 36 37 time(&now); 38 strcpy(temp, ctime(&now)); 39 strncpy(log_time, temp, strlen(temp) - 1); // remove trailing \n from temp 40 fprintf(log_file, "%s: %s", log_time, info); 41 fflush(log_file); 42 43}

TeaRDoWN
Member #8,518
April 2007
avatar

LennyLen: Thanks, but I'm logging mainly to screen but also to file if you quit the game and resume later. The log has been used during development but we've now added a lot of information to the player what happens so it is more a information service than a log system. ;)

GullRaDriel
Member #3,861
September 2003
avatar

Here is something like you want + explanations.

#SelectExpand
1/******** Courtesy of GullRaDriel in his spare time ;-) ******/ 2 3 4#include <stdio.h> 5#include <stdlib.h> 6#include <string.h> 7 8#ifndef true 9 #define true 1 10#endif 11 12#ifndef false 13 #define false 0 14#endif 15 16 17typedef struct LOG 18{ 19 char* text[50]; /* please initialize that fucking pointer array to NULL. */ 20}LOG; 21 22 23 24LOG *new_log() 25{ 26 int it = 0 ; 27 LOG *tmplog = NULL ; 28 29 tmplog = (LOG *)malloc( 1 * sizeof( LOG ) ); 30 if( !tmplog ) 31 return NULL ; /* never forget to check against malloc */ 32 33 for( it = 0 ; it < 50 ; it ++ ) 34 tmplog -> text[ it ] = NULL ; 35 36 return tmplog; 37} 38 39 40int _log(char* text , LOG *mLog ) 41{ 42 char *tmpstr = NULL ; /* a temporary pointer to use for creating a new memory block for text */ 43 int l = 0 ; 44 45 if( !text ) 46 return false ; 47 48 /* instead of manually malloc and sprintf each time, just add it to your log function. */ 49 tmpstr = (char *)calloc( (strlen( text ) + 1 ) , sizeof( char ) ); 50 if( !tmpstr ) 51 return false; 52 53 strcpy( tmpstr , text ); 54 55 /* here is your error, you shift entries, but do not take care of the one who'll be 'forgotten' by your shifting. */ 56 /* So, without doing nothing before, please first FREEEEEEe the last line of your log board */ 57 /* That was the point to fill the array with NULL: we will not double free doing so */ 58 if( mLog -> text[ 49 ] ) 59 free( mLog -> text[ 49 ] ); 60 61 for( l = 49; l > 0; l--) 62 { 63 mLog -> text[ l ] = mLog -> text[ l-1 ]; 64 } 65 /* you want the last entry to be the first in the board. */ 66 mLog -> text[ 0 ] = tmpstr ; 67 68 return true; 69} 70 71int clearLog( LOG **mLog ) 72{ 73 int l = 0 ; 74 75 if( !(*mLog) ) 76 return false; 77 78 for( l = 0; l < 50; l++) 79 { 80 if( (*mLog) -> text[ l ] ) 81 free( (*mLog) -> text[ l ] ); 82 } 83 free( (*mLog) ); 84 return true; 85} 86 87LOG *my_log_data1 = NULL ; /* to test for log that are < 50 lines */ 88LOG *my_log_data2 = NULL ; /* to test for log that are > 50 lines */ 89 90int main( void ) 91{ 92 int it = 0 ; 93 94 my_log_data1 = new_log(); 95 my_log_data2 = new_log(); 96 97 for( it = 0 ; it < 40 ; it ++ ) 98 _log( "test" , my_log_data1 ); 99 100 for( it = 0 ; it < 40 ; it ++ ) 101 _log( "test" , my_log_data2 ); 102 103 clearLog( &my_log_data1 ); 104 clearLog( &my_log_data2 ); 105 106 exit( 1 ); 107}

"Code is like shit - it only smells if it is not yours"
Allegro Wiki, full of examples and articles !!

TeaRDoWN
Member #8,518
April 2007
avatar

Ok, that's like replacing every single line of code...

It would be nice to know what I was doing wrong so I don't make the same mistake again...or do you mean that everything I've posted is bad? :)

Btw, all 50 strings in text[] is set to NULL when starting the program. So I did that.

-------------------------

ARRRRRRGH! >:(

I'm so stupied!!

Found the problem... :P

#SelectExpand
1void logsnd(int id = -1) 2{ 3 char *text = (char*)malloc(sizeof(text)*64); // Wow, this will leak like the economy of General Motors! 4 if (id != -1) 5 { 6 // Let's move it in here instead! FFS!! 7 if (id == 1337) 8 sprintf(text, "Play VO...", id); 9 else if (id == 0) 10 sprintf(text, "Play music...", id); 11 else 12 sprintf(text, "Play sample: %i...", id); 13 14 log(text, LOG_SOUND); 15 } 16 else 17 strcat(mLog.text[0], "Done!"); 18}

The logsnd(int) was made since I have different functions for playing voice overs, effects and music. But I want to log them all. Everytime the logsnd() was accessed it did a malloc but only when id != -1 it actually used the allocated memory by sending the pointer to log().

Sorry guys. :-[ And I guess if the code wouldn't have been so huge I could have posted it all here. But at least I found the problem. Thanks for the support! :)

GullRaDriel
Member #3,861
September 2003
avatar

You still have a memory leak.

See the lines 55 to 50 of the code I posted before.

I told you what was the problem with the code you gave: if you have more than 50 lines of log, you'll leak a line each time you add a new log line.

I can't think of another way to explain you your problem.

"Code is like shit - it only smells if it is not yours"
Allegro Wiki, full of examples and articles !!

TeaRDoWN
Member #8,518
April 2007
avatar

But the first lines of code in my log() will take care of that, if the last log isn't NULL it is cleaned so the second to last can be put in its place:

void log(char* text, int color)
{
  if (mLog.text[LOGS-1] != NULL)
    free(mLog.text[LOGS-1]);
  ...

GullRaDriel
Member #3,861
September 2003
avatar

This part never been in the code you posted before. >:(

"Code is like shit - it only smells if it is not yours"
Allegro Wiki, full of examples and articles !!

LennyLen
Member #5,313
December 2004
avatar

This part never been in the code you posted before.

It was, though not in the original couple of posts.

Though why anyone would ask for help looking for problems in their code, and then omit all the important parts of that code is beyond me.

GullRaDriel
Member #3,861
September 2003
avatar

Yeah, I read the whole topic a new time and saw it was added just after my post.
Thanks for pointing it out, LennyLen. (BTW, are you a special Debian Distro with some chatting ability ? ;-) )

"Code is like shit - it only smells if it is not yours"
Allegro Wiki, full of examples and articles !!

LennyLen
Member #5,313
December 2004
avatar

(BTW, are you a special Debian Distro with some chatting ability ? ;-) )

I wonder if I could sue them for impinging my good name. ;)

torhu
Member #2,727
September 2002
avatar

A bit late to the party, but I can't help myself... :P

You could change the logging function into this:

int log(const char *fmt, ...);

That way you don't need to bother with providing buffers for formatting when you use it. It can call vsnprintf to the do formatting itself.

And instead of using dynamic memory, you could use statically allocated buffers, and truncate strings that are too long. It's a lot cleaner and less bug-prone. Your screen is only so wide anyway. ;)

Go to: