Reading a string of unknown length from user (works once and stops)

Question

Here is my code:

char* get_string()
{
    #define MAX_STRING_LENGTH 1000
    char *input=NULL;
    char  buffer[MAX_STRING_LENGTH];

        fgets(buffer,MAX_STRING_LENGTH,stdin);
        fflush(stdin);
        if((input=realloc(input,strlen(buffer)))==NULL)
        {
            printf("Error allocating memory for string.");
            return NULL;
        }
        strncpy(input,buffer,sizeof(buffer));

    return input;
}

The 1st time I'm calling the function is works OK, the second time it will return garbage and the program exists with some error.

OK so following the suggestions I edited the code:

char* get_string()
{
    #define MAX_STRING_LENGTH 1000
    char *input=NULL;
    char  buffer[MAX_STRING_LENGTH];

        if(fgets(buffer,MAX_STRING_LENGTH,stdin)==NULL)
        {
            printf("Error reading string.");
            return NULL;
        }
        if((input=malloc(strlen(buffer)+1))==NULL)
        {
            printf("Error allocating memory for string.");
            return NULL;
        }
        strncpy(input,buffer,sizeof(input));

    return input;
}

In main I have:

while(1)
{
tmp_arr_ptr=get_string();
printf("%s",tmp_arr_ptr);
}

However I see the same behavior as before.

UPDATE - changed to strcpy(input,buffer); and now it works fine!

`realloc(input,strlen(buffer))` should be `realloc(input,strlen(buffer)+1)`. +1 for `'\0'`. and use `strcpy` — BLUEPIXY, Mar 06 '14 at 23:48
`sizeof(input)` will be the size of the pointer, and not the length of the buffer to which it points. Use `strdup` instead of `malloc/strncpy` as suggested below. — pat, Mar 07 '14 at 00:01
@BLUEPIXY - strcpy did all the difference. I don't understand why to be honest. Any idea? — user34920, Mar 07 '14 at 00:01

score 2 · Answer 1 · edited May 23 '17 at 11:49

2

Your realloc should be to strlen(buffer)+1 to accommodate the NUL at the end of the string.
Whilst it's harmless here, your strncpy should be setting the maximum amount of bytes to copy to the size of the destination buffer not the size of the source buffer.
fflush(stdin) is undefined behaviour - see fflush(stdin) ANSI C
Why bother with realloc if input is always NULL before? Just use malloc(). Or better simply strdup the buffer.
Check the return value of fgets against NULL.

edited May 23 '17 at 11:49

Community

1
1

answered Mar 06 '14 at 23:49

abligh

24,573
4
47
84

Also `strncpy(input,buffer,sizeof(input));` is a problem. – chux - Reinstate Monica Mar 07 '14 at 04:49

score 0 · Answer 2 · answered Mar 06 '14 at 23:49

You're overwriting the end of your buffer because of the terminating NULL. You need to allocate strlen(buffer) + 1 bytes. Also there is no need for strncpy() since you already know you have allocated enough space.

More thoughts: realloc() is awkward for this purpose. For one thing it preserves the original buffer's contents, which could mean an unnecessary copy. Also there is no need to reallocate unless the string is longer than the previous one -- you can reuse the buffer.

Reading a string of unknown length from user (works once and stops)

2 Answers2