1
\$\begingroup\$

I have a simple function that reads character by character from stdin, while resizing the buffer whenever needed. The implementation will only allow 256 characters to be read, but that can easily be modified. Are there an obvious problems with this function? ie. it relies on undefined behaviour. And how can performance be improved?

void scan(char **buffer) {
char *newBuffer;
unsigned char i = 0;
unsigned char size = 1;
*buffer = malloc(16);
(*buffer)[0] = 0;
while (1) {
    (*buffer)[i] = getchar();
    if ((*buffer)[i] == '\n') {
        (*buffer)[i] = 0;
        return;
    }
    if (i >= (size * 16)) {
        size++;
        newBuffer = realloc(*buffer, size * 16);
        *buffer = newBuffer;
    }
    i++;
}
}
\$\endgroup\$
  • \$\begingroup\$ It seems that (if your UCHAR_MAX is 255) this implementation will support reading of 4080 characters, not 256. \$\endgroup\$ – Toby Speight Nov 13 '18 at 8:32
2
\$\begingroup\$

Missing includes

I think you need to include <stdio.h> and <stdlib.h> for successful compilation.

Always check that allocations succeed

Look at

*buffer = malloc(16);
(*buffer)[0] = 0;

If malloc() returns a null pointer, then the assignment to its first element is undefined behaviour. Your program could crash, destroy your system, or (if you're unlucky) appear to work.

Always check that input succeeds

If getchar() returns EOF, we should stop reading. Note that by storing the result in a char, we lose the ability to distinguish EOF from valid input.

Avoid output-only parameters

Why do we return void, and instead write our result to a supplied pointer argument? I could understand accepting an argument if we were to re-use a buffer passed in, but we just discard it. I'd write this as

/* caller must release allocated memory with free() */
char *scan(void)

Be careful of overflow

If an input line reaches 255*16 = 4080 characters, size++ will overflow to zero. Then our realloc() acts as free(), and we find ourselves accessing memory out of bounds. I recommend using size_t for the length of allocated memory - allocation will fail before we reach the limits of size_t.

Consider increasing the allocation increment

This code reallocates every 16 chars. This makes for a lot of allocations for really long lines. One technique to avoid that, without hugely over-allocation for short lines, is to increase the increment as the buffer gets larger. So instead of always increasing by 16, we might double the buffer size instead, or perhaps double it and add 32. There are various strategies that can be used, with different performance implications (though the differences are likely small compared to the overhead of reading input at all).


Improved version

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

/* Read a line from standard input stream.

   Returns a buffer that must be released with free(),
   or a null pointer if none could be allocated.

   The result will end in a newline character if a
   full line was successfully read.
 */
char *scan(void)
{
    size_t capacity = 16;
    char *buffer = malloc(capacity);
    if (!buffer) {
        return NULL;
    }
    size_t i = 0;

    int c;                      /* current input character */
    while ((c = getchar()) != EOF) {
        if (i + 2 > capacity) {
            /* ensure space for c and terminating NUL */
            capacity *= 2;
            char *newbuf = realloc(buffer, capacity);
            if (!newbuf) {
                /* allocation failed - undo the read, terminate string, and return */
                ungetc(c, stdin);
                buffer[i] = '\0';
                return buffer;
            }

            buffer = newbuf;
        }

        /* We have enough space; now store it */
        buffer[i++] = (char)c;
        if (c == '\n') {
            break;
        }
    }

    if (i == 0) {
        /* we didn't read anything */
        free(buffer);
        return NULL;
    }

    buffer[i] = '\0';
    return buffer;
}

Demo

#include <string.h>
int main(void)
{
    char *s;
    while ((s = scan())) {
        printf("%04zu %s\n", strlen(s), s);
        free(s);
    }
}
\$\endgroup\$
  • \$\begingroup\$ re: scan(void). Corner case: when a input error occurs, the code will, if some data was read prior, not return indication of a error. To cope, suggest before if (i == 0) { add if (c == EOF && !feof(stdin)) { i = 0; }. \$\endgroup\$ – chux Nov 13 '18 at 20:44
  • \$\begingroup\$ Like the ungetc(c, stdin); part on allocation failure - have to remember that good detail. \$\endgroup\$ – chux Nov 13 '18 at 20:45
  • \$\begingroup\$ When will getchar() return an EOF? I'm still new to C and I thought that it will simply wait for an input? \$\endgroup\$ – wispi Nov 13 '18 at 22:27
  • \$\begingroup\$ @chux, not sure I understand your first comment - if there's a failure after some input was read, we'll return what we can (which won't end in a newline, so distinguishable from 'good' input). A subsequent call will return NULL. Does the function comment need to be clearer, perhaps? \$\endgroup\$ – Toby Speight Nov 14 '18 at 8:27
  • \$\begingroup\$ @wispi - getchar() will return EOF at the end of the input stream, or when there's any failure when reading. The most likely causes of failures are OS-dependent, so you should consult the manual pages on your own system to increase your understanding. \$\endgroup\$ – Toby Speight Nov 14 '18 at 8:32
1
\$\begingroup\$
  • realloc may fail. In that case, *buffer = newBuffer; without checking would result in a memory leak. Consider

        if (newBuffer) {
            *buffer = newBuffer;
        } else {
            handle_error_as_appropriate;
        }
    
  • Along the lines of the previous bullet, don't throw away valuable information you already obtained (in your case, the input length). Rather than being void, consider returning something useful. The standard Unix convention suggests returning a length of an input (or -1 on error).

  • The function presumes that the stream is infinite. It does not account for EOF of input errors.

  • I do not endorse meddling with input. You never know whether the newline character is significant or not. Refrain from

        if ((*buffer)[i] == '\n') {
            (*buffer)[i] = 0;
    

    Prefer

            (*buffer)[i + 1] = 0;
    
  • size *= 16 seems rather aggressive. Usual factor is 2.

\$\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.