Is there a memory leak in my code? (using pointers to store strings)

Question

#include <stdio.h>
#include <stdlib.h>

int main(void) {
    int x;
    int *in, *begin;
    in = (int *)malloc(sizeof(int));
    begin = in;
    while ((x = getchar()) != EOF) {
        *in = x;
        in++;
        in = (int *)malloc(sizeof(int));
    }
    *in = EOF;
    while ((x = *begin) != EOF) {
        putchar(x);
        begin++;
    }
    free(in);
    return 0;
}

I have a sneaking suspicion that it does.

With this program, I'm trying to store user input of an indefinite size into memory using pointers, as opposed to using char string[255]; fgets(string, sizeof(string)); etc.

EDIT: The program doesn't crash or anything when I run it, I just have a feeling there's memory getting allocated that isn't getting freed.

Answer 1

Yes, the program has a memory leak.

int *in, *begin;
in = (int *)malloc(sizeof(int));  /* allocate space for 1 int, at location "X" */
begin = in;
while ((x = getchar()) != EOF) {
    *in = x; 
    in++;    /* "in" increments address (to location) "X+1" */
    in = (int *)malloc(sizeof(int)); /* address "X+1" is lost as malloc returns  
                                        a different memory location, *not*
                                        necessarily at "X+2". Access to 
                                        previous data other than pointed to by 
                                        "begin" is lost */
}

*in = '\0';  /* this makes probably more senese than assigining EOF here */

There needs to be corresponding calls to free() when you allocate memory.

Also, I don't think the input is stored correctly.

in is never given a continious block of memory to store the data. Instead a single memory location of size to store an int is repeatedly allocated and assigned to in, but we don't really know where this memory is, so all of these allocations are lost since only a single pointer in is keeping track of them.

In other words, the leak consists of repeatedly allocating memory for the size of an int, assigning it to in, and then losing any reference to that location next time through the loop.

Variable begin initially points at the first item entered, but then subsequently trapses through unknown memory as its pointer value is incremented by 1 repeatedly in the output loop.

A better approach would be to allocate a single, larger continuous buffer once at the start and then use it as you increment your in pointer, or to start with a smaller amount but then monitor memory use and realloc() more as needed (but much more overhead to save a few byes of memory).

Also, at the end of your first loop rather than assigning EOF to in, it would make more sense to put in a null character.

Finally, the free(in) call at the bottom of the program frees only a single memory location, none of the other previously allocated memory.

Here's a quickly put together version that works, I tried to make minimal changes to the original code and to keep your code structure intact (I am sure you had your reasons for writing it this way with two loops in the first place) though this could be written much more compactly with just one loop.

Note I initially allocate space for 100 characters, adjust this according to your needs, or alternatively allocate less initially, but then keep track of memory consumption and realloc() more memory as you need (which I think was your initial intention, but just not implemented quite correctly).

int main(void) {
    int x;
    int *in, *begin;
    int *start_loc;
    in = (int *)malloc(sizeof(int) * 100); /* continious space */
    begin = in;
    start_loc = in; /* keep track of start location for final free() call */

    while ((x = getchar()) != EOF) {
        *in = x;
        in++;
    }
    *in = 0;  /* terminator for the input string/data */

    while (*begin != 0) {   /* simplified */
        putchar(*begin);
        begin++;
    }

    free(start_loc);  /* free allocated memory */
    return 0;
}

This could be written without the use of a new variable start_loc (by reusing in for instance) but I chose to write it this way to emphasize the importance of keeping track of the start of your memory allocation and the ability to correctly free memory allocated, so to address your memory leak problem.

Answer 2

When you use malloc to allocate memory, you should make sure that the pointer which point to the memory is not NULL, and after you free the memory, you are better to set the pointer to NULL

Answer 3

Yes you have. Free will deallocate memory for one only integer. You have to call free for every malloc call you have done.

Also to store the characters in a continuous buffer you have to malloc an amount of memory at the beginning and use realloc if the characters you read become more than those initially allocated memory for.

Also don't forget to allocate one more character for the \0 at the end of the string.

When you are done you can call free(buffer) and... Success! No memory leaks!

And the code for it:

/* Start with an initial size */
int size = 128;
char *buffer = (char *)malloc(size + 1);
int i = 0;
while ((x = getchar()) != EOF) {
        buffer[i] = x;
        i++;
        if(i == size){
           /*Do realloc and increase size */
        }
}
buffer[i] = '\0';
/* Do what you want with buffer */
free(buffer);

Answer 4

Levon's answer is correct. You increase the value of in here:

in++;

...but then you reassign it to an address that is arbitrary here:

in = (int *)malloc(sizeof(int));

To achieve what you're trying to do you either need to:

• Allocate a large chunk of contiguous memory in one go, and realloc() whenever it needs to expand (this is what you're trying to achieve), or;
• Use linked lists to navigate through a list of non-contiguous memory addresses (this is what you've actually managed to write).

Answer 5

Of course there is a memory leak, for every call to malloc there sould be a corresponding call to free in your program.

In your program malloc is called several times but there is only one call to free.