Click here to Skip to main content
13,142,065 members (52,507 online)
Rate this:
 
Please Sign up or sign in to vote.
See more:
Hello,
I'm new to C and I was wondering if the following code would leak memory? If so, what do I need to free so it won't leak. Thanks!

#include <stdio.h>
#include <stdlib.h>
char *getcommand(void);
int main(){
    char *cmd;
        while(1){
        cmd = getcommand();
        if (cmd[0] == 'q'){
            printf("exit character has been detected\n");
            return(0);
        }
        printf("the command is %s\n",cmd);
        }
    return 0;
}
char *getcommand(void){
    char x;
    char *buffer=NULL;
    char *tempalloc;
    int count = 0;
    do{
        count++;
        x = getchar();
        tempalloc = (char*) realloc(buffer,count*sizeof(x));
        if(tempalloc == NULL){
            puts("memory could not be allocated!");
            free(buffer);
            exit(1);
        }
        else{
        buffer = tempalloc;
        *(buffer+count) = x;
        printf("%s",buffer);
        }
    }while(x != '\n');
    *(buffer++) = '\0';
    return buffer;
}
Posted 19-Dec-10 14:35pm
Updated 19-Dec-10 14:37pm
v2
Comments
Amit Kumar Tiwari 19-Dec-10 22:26pm
   
Code seems to be right. The advantage is that realloc will preserve the contents of the memory.
Emilio Garavaglia 20-Dec-10 3:02am
   
???? It mess up the buffers!
Rate this: bad
 
good
Please Sign up or sign in to vote.

Solution 2

Having a quick look at your code, I see two problems (may be more ;)) :

First is memory overrun, you do tempalloc = (char*) realloc(buffer,count*sizeof(x)); that gives you a buffer of count bytes, so you can write to it from buffer to buffer + count - 1, while *(buffer+count) = x; writes over the boundary.

Second, when you call printf("%s",buffer); you assume that buffer is a null terminated sequence of character, this is not the case in your code, so printf prints all the character until it found a '0', sure you will have a lot of fancy character before ! And you access characters somewhere in the stack (or not).

I assume you are a student, I encourage you to run your code step by step in a debugger, looking buffer and tempalloc variables, with a pen and piece of paper near the keyboard...

Good "C" exercise, you will enjoy C++ and STL container !
  Permalink  
Rate this: bad
 
good
Please Sign up or sign in to vote.

Solution 1

Your getcommand does a a little mess with the buffers.
While looping in the do statement, buffer is made pointing to a buffer that is enlarged as characters are read.

After exiting the loop, the *(buffer++)='\0' statement in fact shift the buffer pointer by one after placing a '\0' thus giving
|0|_|_|_|_|_|_|_|_|_|_|
   ^buffer

The proper statement should have probably been buffer[count]='\0', giving
|_|_|_|_|_|_|_|_|_|0|
 ^buffer

But beware that your call to realloc should allocate (count+1)*sizeof(x), otherwise you will have no space to store the '\0' character at end.

At this point, by returning buffer, you give the caller the responsibility to free what have been allocated.
You have to call free(cmd) before loping back to a new getcommand, and before returning from main.

int main(){
    char *cmd;
        while(1)
        {
            cmd = getcommand();
            if (cmd[0] == 'q')
            {
                printf("exit character has been detected\n");
                break;
            }
            printf("the command is %s\n",cmd);
            free(cmd);
        }
    free(cmd);
    return 0;
}
  Permalink  
v2
Comments
jean Davy 20-Dec-10 4:44am
   
Emilio, evil is in the detail, arbster1 types *(buffer++) = '\0'; not (*buffer++) = '\0';
Emilio Garavaglia 20-Dec-10 13:37pm
   
Fixed!
Also added a remark to realloc (not allocating enough bytes)
Rate this: bad
 
good
Please Sign up or sign in to vote.

Solution 3

As you're new to C I'd strongly advise against messing about with memory allocation and pointers, it will only end in tears. As you seem to want to read a line from stdin and return it to the caller why not just use one of the functions designed to do this sort of thing, e.g. fgets() ?

Don't worry about "efficiency," the I/O overhead with this sort of program is going to be far greater than any gain you'll get from mucking about with malloc/realloc and free.

One final thing - if you want to return the buffer from the function but don't want to have to mess manually copying arrays, consider using a structure to hold the data:

struct cmd
{
char text[ 80 ];
};

When you return one of these the compiler will generate the code to copy the command text AND it will allocate and release the memory used.

Cheers,

Ash
  Permalink  

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

  Print Answers RSS
Top Experts
Last 24hrsThis month


Advertise | Privacy |
Web02 | 2.8.170915.1 | Last Updated 21 Dec 2010
Copyright © CodeProject, 1999-2017
All Rights Reserved. Terms of Service
Layout: fixed | fluid

CodeProject, 503-250 Ferrand Drive Toronto Ontario, M3C 3G8 Canada +1 416-849-8900 x 100