11
\$\begingroup\$

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?
\$\endgroup\$
  • 1
    \$\begingroup\$ This line seems wrong: input_str = realloc(str, size += 16); because str is undefined. Did you copy and paste incorrectly? \$\endgroup\$ – JS1 Jul 25 '16 at 4:59
  • 2
    \$\begingroup\$ Yep - I had changed str to input_str before I brought the code over. thanks. \$\endgroup\$ – galois Jul 25 '16 at 5:47
  • 5
    \$\begingroup\$ realloc does not free the pointer that was passed to it in case of failure. Thus you have two possible memory leaks there. \$\endgroup\$ – Daniel Jour Jul 25 '16 at 10:30
  • 1
    \$\begingroup\$ Neither answer has addressed this concern yet, but in C it is perfectly fine to leave it up to the calling function to free the returned pointer. However, having size in the calling signature would make me as a user think that is the maximum number of bytes read, as opposed to the initial buffer size, since you shrink the buffer to len in your return. \$\endgroup\$ – Adam Martin Jul 25 '16 at 16:10
21
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Note: OP's code does not include '\n'in the return buffer. getline() includes it. \$\endgroup\$ – chux - Reinstate Monica Jul 25 '16 at 15:26
  • \$\begingroup\$ @chux Good point. Though it is quite simple to fix that. \$\endgroup\$ – mleyfman Jul 25 '16 at 18:40
  • \$\begingroup\$ getline isn't part of standard C. It's an extension, provided by compilers like GCC and mandated by POSIX, but not mandated by the C standard. I don't think it's available on Windows. \$\endgroup\$ – user2357112 supports Monica Jul 25 '16 at 20:22
  • \$\begingroup\$ @user2357112 I'd file functionality under Windows to be "special behavior" :) \$\endgroup\$ – mleyfman Jul 25 '16 at 22:26
9
\$\begingroup\$

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)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.