Taking arbitrary length input in C

Question

Instead of having to use something like char buf[100500] and hope that no possible user input could be longer than 100500 bytes, I decided to make the following function:

char* input (FILE* in, size_t size)
{
    char * input_str = NULL;
    int c;
    size_t len = 0;

    /*initial allocation*/
    input_str = malloc(size);
    if (!input_str) return NULL;

    while ((c = fgetc(in)) != EOF && c != '\n')
    {
        input_str[len++] = c;

        /*allocate more room if needed*/
        if (len == size)
        {
            input_str = realloc(input_str, size += 16);
            if (!input_str) return NULL;
        }
    }

    input_str[len++] = 0;

    return realloc(input_str, len);
}

My questions about this code:

• Is leaving it up to the calling function to free memory a bad idea?
• Is there a way to increase performance for this (memory pool, etc)? I don't like making so many syscalls...
• In general, is there a better way to go about this?

Answer 1

A simple improvement to improve performance by reducing the number of reallocs is to start with a decently sized buffer and when it fills up, grow it by a multiple of the size. For example, you could double the size when it fills up.

Of course this approach is less efficient memory wise, but in order to mitigate that, we can multiply by a smaller multiple (like 1.5x) and/or start with a smaller base size. If you don't care much about memory, you can up the multiple and up the base amount.

For truly enormous files, you will want to chunk your data and process it by chunk instead trying to load it into memory all at once. Another possibility is to just process it as it comes in via an on-line algorithm.

On a side note, it seems like you are reimplementing getline()'s functionality, so unless you need some special behavior, I'd recommend using that instead.

Answer 2

Functionality

Error: input(stdin, 0) could quickly return NULL as code incorrectly assumes a NULL return only means out-of-memory.

// if (!input_str) return NULL;
if (!input_str && size > 0) return NULL;

Error: Memory leak @Daniel Jour. Should realloc() fail, code should free input_str.

        size = size ? size*2 : 1;
        void *new_ptr = realloc(input_str, size);
        if (new_ptr == NULL) {
          free(input_str);
          return NULL;
        }

Weakness: Code lacks EOF detection. If only '\n' or EOF is read, both cases return a string of "". Code needs to differentiate. Various approaches exists. With this code, recommend returning NULL when only EOF was read. Calling code could distinguish between OOM and EOF occurrence, by calling feof()/ferror().

Memory leak with return realloc(input_str, len); on rare failure.

 // return realloc(input_str, len);
 void *new_ptr = realloc(input_str, len);
 return new_ptr ? new_ptr : input_ptr;

Soap box

Disagree with unlimited input strategy as it gives external input the ability for code to consume insane amount of resources without control - ripe for hackers. Better to pass in an upper bound.

Design

size parameter seems of little use. It only provides an initial size and is ignored for subsequent re-allocations. Recommend removing it. Just start with some small size.

A better use would be to use size_t *size_read to report that number of characters read. This has an advantage over using strlen() on the return value as 1) the length does not need O(n) computation and 2) handles the corner case should code read null characters which strlen() will not detect.

IMO, code should not attempt to read unless the needed memory has first been successfully allocated. Also on allocation failure, perhaps it would be better to return the partial read line. Of course, conveying that information is another issue. IAC, this allows the calling routine more control on error handling.

Performance

Use exponential growth in re-allocation rather than linear. This is well answered by @mleyfman

With exponentiation strategy, it makes even more sense then to do a final "right-size " re-allocation, which OP did. With linear +16 re-allocation, it has reduced value to the point that it is unnecessary.

Minor: Check against the more common '\n' first.

// while ((c = fgetc(in)) != EOF && c != '\n')
while ((c = fgetc(in)) != '\n' && c != EOF)