will this leak memory?

Question

5.00/5 (3 votes)

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

arbster1

Updated 19-Dec-10 14:37pm

HimanshuJoshi

v2

Add a Solution

Comments

Wild-Programmer 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!

3 solutions

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 !

Posted 19-Dec-10 22:40pm

jean Davy

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

Posted 20-Dec-10 3:48am

Aescleal

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

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

Emilio Garavaglia · Accepted Answer · 2010-12-19T21:02:00

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.

C#

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;
}