leaking memory and possible malloc error

Question

I am building an array and I want it to be a fixed size so that as I read in a file it only stores the last 10 commands. The file seems to read in correctly and it looks right when I print it but for some reason my memory is not getting freed. MAX is set to 1000 and historySize is read earlier from the user. I ran valgrind on my code and when the calls to these functions are commented out I do not have any leaks.

I have a char ** history under my #includes

Here is my code

void setupHistoryFile()
{
    char string[MAX];
    FILE *fp;
    int len;
    int pos = 0;
    fp = fopen(".ush_history","r");
    if(fp == NULL)
    {
        //create the file
        fp = fopen(".ush_history","w");
    }
    else
    {
        history = (char**)malloc(historySize * sizeof(char*));//setup history file
        fgets(string,MAX,fp);
        len = strlen(string);
        if(string[len-1]=='\n')
            string[len-1]='\0';
        while(!feof(fp))
        {
            if(history[pos] != NULL)
            {
                free(history[pos]);
                history[pos]=NULL;
            }
            history[pos] = (char*)malloc((strlen(string)+1) * sizeof(char));
            //printf("Should be copying %s\n",string);          
            strcpy(history[pos], string);           
            pos++;
            pos = pos % historySize;
            fgets(string,MAX,fp);
            len = strlen(string);
            if(string[len-1]=='\n')
                string[len-1]='\0';
        }
    }
    fclose(fp);
}

I do have a function that cleans the history and it looks like this

void cleanHistory()
{
    int i;
    if(history != NULL)
    {
        for(i=0;i<historySize;i++)
        {
            free(history[i]);
            history[i] = NULL;
        }
        free(history);
        history = NULL;
    }
}

Are you sure your deallocation function cleanHistory() is getting called on the very same pointer array to which you allocated the memory? You can put breakpoints in cleanHistory() and check if this functions gets called at all. — Alok Save
– Alok Save, Commented Jan 31, 2012 at 7:27
I have a char** history under my #includes and I am able to print the contents of it in main after calling the function. — samack
– samack, Commented Jan 31, 2012 at 7:30

Some programmer dude · Accepted Answer · 2012-01-31 07:46:15Z

7

When you allocate memory with malloc, the chunk of memory allocated is not initialized. That means that if you do something like history[pos] != NULL might actually be true even if you haven't put anything there.

To be certain that the allocated memory is initialized, either use calloc or memset.

Edit To be more specific, this part of your code will behave badly:

if(history[pos] != NULL)
{
    free(history[pos]);
    history[pos]=NULL;
}

If you are not lucky, history[pos] will contain some old data, which means you will try to free something you have not allocated.

As a small side-note, you should be looping while fgets doesn't return NULL. As it is now you don't check for errors from fgets. Something like this:

while (fgets(...) != NULL)

Then you don't need the double calls to fgets, and you will stop looping on both error and end of file.

edited Jan 31, 2012 at 7:46

answered Jan 31, 2012 at 7:28

Some programmer dude

411k36 gold badges420 silver badges655 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Secure Over a year ago

Don't use calloc or memset to initialize an array of pointers. Use a loop and set each array element to NULL. The binary representation of NULL may be all bits 0 on most systems, but this is not required.

Some programmer dude Over a year ago

@Secure While technically true, do you know of any modern and commonly used computer (outside old mainframes) where NULL is not the same as 0?

Secure Over a year ago

It is usually a choice of the compiler implementation, not of the machine. I had an old x86 compiler years ago that used all bits 1 for NULL pointers. So in theory, the gcc developers could decide to use it in the next version. Or to use a different NULL representation for each coming version. It would be slightly crazy, but still conforming. However, when confronted with the choice to either save some minor typing or to stay fully portable, I usually prefer the latter.

Cătălin Pitiș · Accepted Answer · 2012-01-31 07:31:08Z

0

You allocate memory for history, but you don't initialize it. That means at first pas through history, it might happen that history[pos] is not allocated, nor NULL, and you'll try to deallocate unallocated memory.

answered Jan 31, 2012 at 7:31

Cătălin Pitiș

14.4k2 gold badges41 silver badges63 bronze badges

Collectives™ on Stack Overflow

leaking memory and possible malloc error

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related