Mdthbs

I've written code in C for reading a string of any length, and printing the string. It's one of my first C programs. I posted the program as part of a Stack Overflow question, and received a comment:

... there are many things wrong with your C version.

I'd really like to know what is wrong with the function I have written. Suspects: EOF handling, calling dynamic memory allocation functions too many times.

#include <stdlib.h>

#include <stdio.h>



/* Asks the user for string input.

 * Returns a pointer to the string entered by the user.

 * The pointer must be freed.

 */

char* input()

{

    char *s = malloc(sizeof(char));  /* For the null character */

    if (s == NULL) {

        fprintf(stderr, "Error: mallocn");

        exit(1);

    }

    s[0] = '';



    /* Read characters one by one */

    char ch;

    size_t s_len = 0;

    while((ch = (char) getchar()) != 'n') {

        if (ch == EOF) {

            exit(0);

        }

        s_len++;

        s = realloc(s, (s_len * sizeof(char)) + sizeof(char));

        if (s == NULL) {

            fprintf(stderr, "Error: reallocn");

            exit(1);

        }

        s[s_len - 1] = ch;

        s[s_len] = '';

    }

    return s;

}



int main()

{

    printf("Name: ");

    char *s = input();

    printf("%sn", s);

    free(s);

    return 0;

}

/* Asks the user for string input.

 * Returns a pointer to the string entered by the user.

 * The pointer must be freed.

 */

Slightly misleading in that this function doesn't ask for input. (As written it is not responsible for printing the prompt.)

Perhaps also should clarify the intended behavior:

• If there is input that is not terminated by a newline, is that string returned?


• When input is terminated with a newline, is the newline preserved in the returned string?

char* input()

{

    char *s = malloc(sizeof(char));  /* For the null character */

In C, char by definition is 1 byte. sizeof (char) is unnecessary and adds visual noise.

    if (s == NULL) {

        fprintf(stderr, "Error: mallocn");

        exit(1);

It's usually considered poor behavior if calling a library function causes the entire program to terminate. You should return an error value (e.g. NULL) and leave that decision up to the caller.

    }

    s[0] = '';



    /* Read characters one by one */

    char ch;

ch must be an int. getchar returns an int precisely so that it can return a value outside the range of char to indicate failure (i.e., the EOF value). Otherwise you would not be able to distinguish EOF from a legitimate byte value.

    size_t s_len = 0;

    while((ch = (char) getchar()) != 'n') {

        if (ch == EOF) {

It's more idiomatic to check for EOF in the loop condition and then check for character values inside the loop body:

int ch;

while ((ch = getchar()) != EOF) {

    if (ch == 'n') {

        ...

Checking for EOF in the loop condition allows you to safely truncate ch to char throughout the entire loop body, which makes it a bit easier to reason about.

            exit(0);

Same thing here about terminating the program. It's especially weird here since this exits with a success code and doesn't return the string to the caller.

        }

        s_len++;

        s = realloc(s, (s_len * sizeof(char)) + sizeof(char));

Never do x = realloc(x, ...). If realloc fails and returns NULL, you will have lost the old value of x and will be unable to free it, resulting in a memory leak. You instead should use a temporary variable:

char* newBuffer = realloc(s, ...);

if (newBuffer == NULL) {

  ...

}

s = newBuffer;

Again, sizeof (char) is noise. You additionally should generally avoid sizeof (Type); it's not robust if the types change since you would need to edit more places (and neglecting to do so could lead to silent buffer overflows and introduce security vulnerabilities). It's better to use sizeof expression where expression is based on the corresponding variable. For example, if you want the code to be robust if the type of s changes (e.g. if you wanted to adapt the code to support wchar_t), you should do:

char* newBuffer = realloc(s, (s_len + 1 /* NUL */) * sizeof *s);

...

Finally, calling realloc for every byte read is grossly inefficient. A better approach is to maintain a buffer where you keep track of its allocated size and to grow it exponentially (e.g. doubling in size) whenever you need more space.

        if (s == NULL) {

            fprintf(stderr, "Error: reallocn");

            exit(1);

Same thing about terminating the program. Also should do free(s); along this path.

int main()

{

    printf("Name: ");

I/O is normally buffered. You should call fflush(stdout) afterward to ensure that the prompt is printed to the screen before waiting for input.

Realistically, you would want to check that the input characters are acceptable to your program.