8
\$\begingroup\$

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: malloc\n");
        exit(1);
    }
    s[0] = '\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: realloc\n");
            exit(1);
        }
        s[s_len - 1] = ch;
        s[s_len] = '\0';
    }
    return s;
}

int main()
{
    printf("Name: ");
    char *s = input();
    printf("%s\n", s);
    free(s);
    return 0;
}
\$\endgroup\$
15
\$\begingroup\$
/* 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: malloc\n");
        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] = '\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 (technically unsigned 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: realloc\n");
            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.

\$\endgroup\$
  • \$\begingroup\$ Regarding getchar(): after checking that the character is not \n or EOF, can I safely cast the int to a char? I mean at some point I would have to place chars into the char array that will be returned. \$\endgroup\$ – Flux Sep 13 at 16:48
  • 1
    \$\begingroup\$ @jamesdlin My reading of the standard is that you can safely cast it to unsigned char, but you can't safely cast it to signed char or char. Reading with getchar into a char array is surprisingly harder than it should be. You could write ((unsigned char *)s)[pos] = ch, but it isn't even guaranteed that char can represent as many values as unsigned char – despite the fact that fgets and friends take char * pointers, not unsigned char *. It's not something that a beginner should have to think about, but if you're a stickler for portability then technically you have to... \$\endgroup\$ – benrg Sep 14 at 3:30
  • 1
    \$\begingroup\$ @benrg Oops. Yes, fgetc/getchar returns a char as an unsigned char, and you would need to alias s as you describe. That said, I think casting the result of fgetc/getchar to unsigned char and then char usually should be good enough (the unsigned char to char cast is implementation-defined, but I think you'd have to find a very pathological implementation that doesn't preserve the bits). Additionally, a pathological implementation could define sizeof (int) == 1 with CHAR_BIT >= 16, in which case the usual idiom for checking for EOF isn't strictly correct either. =/ \$\endgroup\$ – jamesdlin Sep 14 at 4:37
  • 1
    \$\begingroup\$ @CodyGray If it needs to be expressed in terms of the size of each element, then sizeof *s (or equivalent) should be preferred. Additionally, if your intent is to allocate a number of bytes, it's seems silly to multiply a length by the size of a byte, and a byte is what char is. \$\endgroup\$ – jamesdlin Sep 15 at 2:45
  • 1
    \$\begingroup\$ @jamesdlin I often say int is returned to distinguish typically 1 of 257 conditions: an EOF or unsigned char. \$\endgroup\$ – chux - Reinstate Monica Sep 28 at 8:44
1
\$\begingroup\$

Avoid trying to combine assignment and compare in the same statement. The cast within the statement makes it even hard to read.

Also, handle EOF with getchar. You can get EOF with redirected input.

Instead of this:

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

    . . .
}

This:

int value = getchar();
ch = (char)value;
while (value != EOF and ch != '\n') {
    ...

    value = getchar();
    ch = (char)value;
}
\$\endgroup\$
  • 5
    \$\begingroup\$ Calling getchar() from two places is worse, in my opinion. while( (ch = getchar()) != EOF ) { } is correct idiomatic C to scan a file. \$\endgroup\$ – jcupitt Sep 14 at 13:11
  • \$\begingroup\$ This is way more error-prone (there's duplicate code, and it's easy to accidently compare ch != EOF). It'd be much better to restrict the cast version to be within the loop body. If you really want to avoid the assignment in the loop condition, IMO it's be better to have while (true) { int ch = getchar(); if (ch == EOF) break; ... \$\endgroup\$ – jamesdlin Sep 14 at 21:50
1
\$\begingroup\$

String vs. line

Read string of any length in C

User input in C is better described as reading a line than a string.

A string is a contiguous sequence of characters terminated by and including the first null character.

A text stream is an ordered sequence of characters composed into lines, each line consisting of zero or more characters plus a terminating new-line character. Whether the last line requires a terminating new-line character is implementation-defined. C11dr §7.21.2 2

After reading a line, input is converted to a string by appending a null character. The appended null character is part of the string, but not user input.

Null characters

The tricky bit is what happens when reading input that itself contains a null character? Rare, but not prevented in code.

OP's approach of char* input() fails to always provide unambiguous length information.

With char *s = input();, code does not know if the first null character encountered in s is the appended one or a read one.

Conveying length resolves this issue.

char* input(size_t *sz) { 
  *sz = 0;
  ....
  *sz = s_len;
  return s;
} 

size_t sz = 0;
char *s = input(&sz);

printf("Length of input:%zu\n", sz);

Consider defensive programming.

Reading strings of any length allows a nefarious user to overwhelm system memory resources with an long input - perhaps gigabytes.

The nature of user input, IMO, should have a generous upper bound. In which case, code can simply use fgets() (not withstanding the above null character issue.)

#define USER_NAME_SZ 4096

char name[USER_NAME_SZ];
if (fgets(name, sizeof name, stdin)) {
  if (strlen(name) >= USER_NAME_SZ - 1) {
    fprintf(stderr, "Hostile input detected\n");
    exit(-1);
  }

Exit on end-of-file?

OP's code exits the program on end-of-file - a rude thing for input() to do.

if (ch == EOF) {
  // exit(0);
  if (feof(stdin)) {
    if (s_len > 0) return s; // Return what was read
    free(s);
    return NULL;             // Let caller cope with end-of-file.
  }

EOF: end-of-file or error?

Rarely getchar() returns EOF due to an input error.

If the stream is at end-of-file, the end-of-file indicator for the stream is set and getchar returns EOF. If a read error occurs, the error indicator for the stream is set and getchar returns EOF.

It is better to distinguish using feof() than ferror().

So rather than simply exit on EOF, code may want to distinguish.

if (ch == EOF) {
  // exit(0);
  if (feof(stdin)) {
    if (s_len > 0) return s; // Return what was read
    free(s);
    return NULL;
  }

  // This is trickier - what to due on input error?
  // Different schools of thought exist.
  // Usual, like fgets(), return NULL
  free(s);
  return NULL;
} 


char *s = input();
if (s) {
  puts(s);
} else {
  if (feof(stdin)) puts("end-of-file");
  else puts("end-of-file");
}
\$\endgroup\$
0
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Can you expand on what you mean? Are you suggesting ensuring that if getchar() doesn't return EOF the they should ensure it's only 8 bits? Isn't that guaranteed by getchar()? If that's not what you mean, what do you mean? \$\endgroup\$ – user1118321 Sep 14 at 17:05

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.