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++;

}

}

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] = '';

                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] = '';

    return buffer;

}

Demo

#include <string.h>

int main(void)

{

    char *s;

    while ((s = scan())) {

        printf("%04zu %sn", strlen(s), s);

        free(s);

    }

}

• 
  

  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.