Seg fault on my own version of getline

Question

I'm trying to make a simple version of getline. It should read a line in from stdin, reallocating the size of the buffer as necessary. It should also return the number of characters read. It takes a char ** so that the reallocated buffer can be later freed. Why am I getting a segfault?

Heres my version:

int get_input_line(char **buff, int start_size) {
char c;
int stop = 0, length = 0, k = start_size;
while(!stop) {
    if(length > k) {
        k += 50;
        buff = (char *)(realloc(buff, start_size + 1));
    }
    c = getchar();
    if(c == '\n'){
        stop = 1;
    }
    buff[length] = c; 
    length++;
}
return length;
}

And here's the call:

char *buff = (char *)(malloc(50 + 1));
get_input_line(&buff, 50);
printf("%s", buff);

Answer 1

You're not detecting EOF reliably. You need to save the result of getchar() in an int and not a char. And you should not try to store EOF in your buffer.

You're not checking your memory allocations.

You're not null terminating the output string, so the printf() in main() may crash.

You're confusing someone (maybe me, maybe the compiler, maybe yourself) by allocating 51 bytes and telling the function that it only has 50 bytes to play with.

And, most particularly, you need to be using *buff at most points inside the function, including, in particular, when adding a character:

(*buff)[length++] = c;

You really should be paying more attention to all those compiler warnings. If your compiler isn't giving you any, get a better compiler (or turn on the warning flags - but you should be being shrieked at by the compiler in its default mode).

Also, you are miscalling realloc() on three grounds. One is the *buff issue. The second is that you want the size to be k, not start_size + 1. The other is that you are assigning the result to the input parameter. This is a 'no-no' because if the allocation fails, you've lost your pointer to the previously (and still) allocated data. Always use the idiom:

void *new_data = realloc(old_data, new_size);
if (new_data == 0)
    ...deal with out of memory error...
else
{
    old_data = new_data;
    old_size = new_size;
}

Applied to your code, that means:

char *new_buff = (char *)realloc(*buff, k); // NOT start_size+1!!!
if (new_buff == 0)
    ...deal with out of memory error...
else
    *buff = new_buff;

There are those who argue against the cast on malloc() and realloc() and calloc(); there are those who prefer the casts present. There are arguments on both sides, of differing degrees of validity. I prefer the cast - I respect those who prefer no cast. We reach our different conclusions for different reasons.

I have not studied the code for other 'off-by-one' errors. I suspect that there may be several of those, too.