Dynamic fgets in C

Question

I wanted to make a function that will dynamically retrieve a line from a stream to a buffer. This function just needs to take in the char* and the stream to read from. It'll keep allocating a bigger buffer until it can read all the data into the rtr variable (terminated on new line). Any ways I can improve this would be amazing.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

char* dynamic_fgets(char** rtr, FILE* stream) {
    int bufsize = 1024; //Start at 1024 bytes
    char* buf = (char*) malloc(bufsize*sizeof(char));

    if(buf == NULL) {
        perror("Couldn't allocate memory for buf in dynamic_fgets\n");
    }

    do {
        fgets(buf, bufsize, stream);
        *rtr = realloc(*rtr, strlen(buf)+strlen(*rtr));
        if(*rtr == NULL) {
            perror("Couldn't allocate memory for *rtr in dynamic_fgets\n");
        }

        *rtr = strncat(*rtr, buf, bufsize);
        bufsize *= 2;
    } while(buf[strlen(buf)-1] != '\n');

    return *rtr;
}

int main(int argc, char** argv) {
    char* buf = (char*) malloc(sizeof(1));
    strncpy(buf, "\0", 1);
    printf("Input:  ");

    dynamic_fgets(&buf, stdin);

    printf("Output:  %s", buf);

    return 0;
}

Answer 1

Code runs into undefined behavior (UB)

Code fails to test the return value of fgets(). Without knowing the return value is not NULL, strlen(buf) can get called on 1) uninitialized memory or 2) memory in an undefined state or 3) buf[] from the previous fgets() call.

// UB possible
fgets(buf, bufsize, stream);
*rtr = realloc(*rtr, strlen(buf)+strlen(*rtr));

// Better
if (fgets(buf, bufsize, stream)) {
  ... strlen(buf) ...

Infinite loop

Should user input end with a non-'\n' (think piped input), do loop does not stop before UB kicks in.

Exploitable code

dynamic_fgets(&buf, stdin); allows an external user make the program use excessive resources. There is no upper bound. Defensive coding does not allow memory consumption to go unchecked from potential hostile users or pathological file input.

IMO, dynamic_fgets() , deserves an upper bound parameter. Once exceeded, the routine should not allocative more memory and instead return an error indication.

Code is fooled with '\0'

How robust do you want your code?

In hostile cases, fgets() may read a null character and fgets() treats it no different than any other non '\n' character. A following strlen(buf) can stop on the usual null character appended by fgets(), yet it will first stop on a read null character. strlen(buf) could be 0 due to such input.

Further this code is then readily exploitable due to buf[strlen(buf)-1] by entering a null character as the first input character. The code is the same as buf[SIZE_MAX] an access outside the allocation and thus UB. At a minimum, do not code buf[strlen(buf)-1] when buf[0] == 0 is possible.

size_t vs. int

Minor: size_t is the right width for array sizing. It is the type return by strlen() and accepted by *alloc(). int can be insufficient for long strings.

Answer 2

• Make up your mind. dynamic_fgets returns the same information via return value (return *rtr) and the in-out parameter (char ** rtr). Chose one.

• Do not cast malloc return value. In C it is redundant, and may cause hard-to-find bugs.

  If, for any reason, you feel you need to cast it, be consistent and cast realloc as well.

  Ditto for sizeof(char). It is 1 by definition; if you still want to spell it out in a call to malloc, do so for realloc.

• *rtr = realloc(*rtr, ....) is dangerous, for it leads to memory leak. Shall realloc fail, the memory pointed by *rtr prior to the call is lost. Consider instead

  char * tmp = realloc(*rtr, ....);
  if (tmp == NULL) {
      // whatever recovery strategy you may come up with, e.g. free(*rtr);
      ....
      return error_indication;
  }
  *rtr = tmp;
  .... // Business as usual
  

• Every call to dynamic_fgets allocates 1024 bytes for buf, which is never freed. 1024 bytes per call leaked.

• An unconditional reallocation looks strange. Chances are that what fgets have read is fitting into an allocated space just fine. Consider testing for \n first, and realloc only if necessary.

• strncat(*rtr, buf, bufsize) must find a terminating \0 in a memory pointed by *rtr, which leads to quadratic time complexity. You should track where the terminator was placed by the previous iteration.

• buf is unnecessary. You may read directly into *rtr (plus offset, see the above bullet), thus sparing a copy.

Answer 3

I suggest using the POSIX getline() function as it does what the posted code is trying to do. However, it does take a third parameter (a pointer to a variable to receive the length of the read line). It also returns the number of bytes read (or an EOF indicator).

Answer 4

Bug: buffer overflow

You only ever allocate buf once, with size 1024, here:

int bufsize = 1024; //Start at 1024 bytes
char* buf = (char*) malloc(bufsize*sizeof(char));

But later, you double bufsize on every iteration through the loop, without also doubling the allocation of buf.

Therefore, on the second iteration of the loop, when you run this line:

    fgets(buf, bufsize, stream);

you will overflow your buffer, because buf has 1024 bytes, but bufsize will be 2048. As user vnp mentioned, you don't really even need the buf variable if you just read to the end of *rtr and keep track of the number of bytes read so far.

Shlemiel the painter

There is a funny story about a painter who paints slower and slower every day. The punchline:

"I can't help it," says Shlemiel. "Every day I get farther and farther away from the paint can!"

This story is actually about concatenating strings. In your loop, there are the following operations:

strlen(buf) x2
strlen(*rtr)
strncat(*rtr)

You can't avoid one call to strlen(buf) but the calls involving rtr can be avoided by simply tracking the length of rtr in some variable.

Answer 5

One more thing not mentioned yet. Dont put command line arguments if you dont use them in the code. It is just misleading since you dont use argc and argv. consider to use simply int main (void)