Problems with memory allocation [closed]

Question

Closed. This question does not meet Stack Overflow guidelines. It is not currently accepting answers.

---

Questions asking for code must demonstrate a minimal understanding of the problem being solved. Include attempted solutions, why they didn't work, and the expected results. See also: Stack Overflow question checklist

Closed 8 years ago.

Improve this question

#include <stdio.h>
#include <stdlib.h>
int main ()
{
    char *buffer=malloc(1);
    char *temporary=malloc(1);
    int length=0;
    signed int character;
    for(length=0;(character = getchar())!=EOF;length++)
    {
        buffer[length]=character;
        if((length==0) || ((length%1000)==0))
        {
            temporary=realloc(buffer,length+1001);
            if(temporary!=NULL)
            {
                buffer= temporary;
            }
            else
            {
                printf("Error: Out of memory!\n");
                free(temporary);
                free(buffer);
                return 1;
            }
        }
    }
    buffer[length]='\0';
    length-=1;
    while(length>=0)
    {
        printf("%c",buffer[length]);
        length--;
    }
    printf("\n");   
    free(buffer);
    //////////////free(temporary); //( HERE)
    return 0;
}

My program should read a string or a input textfile, store it, and then print it out backwards which is working. I just don't understand why I can't free the temporary value at end. It's failing with a double free error.

I would like to know why I cant put a free(temporary) where I wrote "here".

If you want to point out another problem in my code i'm happy to learn something.

Answer 1

When this code executes the line

buffer = temporary;

two things happen. First, the memory that buffer was holding is "leaked"; you no longer have that address to pass to free(). Second, buffer and temporary now both point to a single piece of memory allocated by malloc() or by realloc(). Thus, when you call free() on either buffer or temporary, you have freed that chunk of memory; to call free() again would be a double free error.

I suggest something like this:

if (length >= buffer_size)
{
    const size_t new_size = buffer_size + 1000;
    char *temp = realloc(buffer, new_size);
    if (!temp)
    {
        free(buffer);
        /* handle the error in some way */
    }
    buffer = temp;
    buffer_size = new_size;
}

Also, it is best practice to actually check for errors. malloc() and realloc() can fail.

Also, there isn't much point in allocating a single byte of memory for each buffer. You might as well start out by allocating 1000 bytes or whatever.

Also, the way you check to see if the buffer needs realloc() seems needlessly tricky. I would suggest keeping a variable that tracks how long the buffer is, and call realloc() when the buffer is not long enough, and then store into the buffer after the realloc().

EDIT: I think you got a bit confused about temporary. Look at the code I put and you will see that I put the temp variable inside the block of code of the if statement. It's more obvious how long temp will hold a useful value (not very long). Declaring temporary at the outer scope, and also calling malloc() to put a byte into it, is just confusing.