Click here to Skip to main content
Rate this: bad
good
Please Sign up or sign in to vote.
See more: C
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
Edited 19-Dec-10 14:37pm
v2
Comments
Emilio Garavaglia at 20-Dec-10 3:02am
   
???? It mess up the buffers!
Amit Kumar Tiwari at 19-Dec-10 22:26pm
   
Code seems to be right. The advantage is that realloc will preserve the contents of the memory.
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 Wink | ;) ) :
 
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 at 20-Dec-10 4:44am
   
Emilio, evil is in the detail, arbster1 types *(buffer++) = '\0'; not (*buffer++) = '\0';
Emilio Garavaglia at 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)



Advertise | Privacy | Mobile
Web02 | 2.8.141022.2 | Last Updated 21 Dec 2010
Copyright © CodeProject, 1999-2014
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