Is my code okay or will it fail in some occasion?

Question

I recently learned about the keyboard buffer and how important it is to clean it after getting input from the user. So I made this function to handle that:

// Get input from user
void get_input(char *buff, int size)
{
    fgets(buff, size, stdin);
    int newlineIndex = strcspn(buff, "\n");

    /* If no newline is found, fgets stopped at `size` and
    didn't read all input. Keyboard buffer is dirty */
    if (newlineIndex == strlen(buff))
    {
        // Clear keyboard buffer
        int c;
        while ((c = getchar()) != '\n' && c != EOF);
    }
    else
    {
        // Remove newline from buff
        buff[newlineIndex] = '\0';
    }
}

Here, buff is where you want to store the input and size is sizeof(buff). After using fgets to read input from the user, I then look for the \n character left by fgets with strcspn(). If newlineIndex == strlen(buff), the \n newline wasn't found, which means that the user has typed more characters than size, so I proceed to clear the keyboard buffer with:

int c;
while ((c = getchar()) != '\n' && c != EOF);

else, the keyboard buffer didn't get dirty, so I remove \n from the user input to handle with it more easily as a string later.
Is my code okay or am I missing something? Is there a more efficient way to do it?

See also, for example: https://stackoverflow.com/a/26081123/10871073 — Adrian Mole, May 01 '22 at 18:25
`newlineIndex == strlen(buff)` is equivalent to `buf[newlineindex] == '\0'`, which should save you a string traversal. — M Oehm, May 01 '22 at 18:26
The code checks for newline present at the end of the input string and if not, clears any remaining input. It's a poor strategy: it's usually a sign that the buffer needs to be bigger, not that you should discard input. — Weather Vane, May 01 '22 at 18:34
@MOehm : Those two expressions are not equivalent. Not sure what you are trying to say there. — Clifford, May 01 '22 at 18:40
@Clifford: Oh, I was somehow waiting for that comment. They are equivalent in the context of OP's code, where `newlineindex` was arrived at via `strcspn`, so it is guaranteed that there are no null characters in between. I was trying to say that if there is no newline in the string, the character at `newlineindex` must be the null terminator, so it is wasteful, but not per se wrong, to call `strlen` here. — M Oehm, May 01 '22 at 18:43
@Adrian Mole Yeah I tried it and it worked. As cleaning the keyboard buffer was a new discovery for me, I just wanted to make sure I wasn't potentially messing anything up. Thanks for the feedback — danilo, May 01 '22 at 19:14
@M Oehm That's true! Thank you, I'll put it in my code and edit it in the question — danilo, May 01 '22 at 19:19
@Weather Vane What do you mean? I'm cleaning the rest of the input from the keyboard buffer to be able to get input from the user again later on the code. Should I create a new buffer with setbuf() instead? — danilo, May 01 '22 at 19:23
I am saying that instead of throwing input away, you should use a bigger buffer. That is how `getline()` does it. — Weather Vane, May 01 '22 at 19:26
Don't change the code in the question! It may make existing answers nonsense. You are asking if your code is OK - if you change it to make it OK from posted suggestions or answers, any answers pointing out any flaws will be rendered useless and possible nonsense. It also devalues the question as a community resource because others will learn nothing from it. Those taking the trouble to post such answers will not thank you. — Clifford, May 01 '22 at 19:31
If you wanted to get really fancy, you might also trim leading/trailing whitespace ;-) — Clifford, May 01 '22 at 19:43
@Weather Vane I see. That's also a new function to me, but it seems way more powerfull than fgets. Thanks for the heads up! — danilo, May 01 '22 at 19:50
@Weather Vane True about "That is how getline() does it.", yet without an upper bound, code gives the user the opportunity to overwhelm memory resources with nefarious or pathologically input. In a well behaved environment, `getline()` is OK. Setting an upper bound size like OP did here is a reasonable defensive step as long as it is generous:, like 2x the expected max input size. — chux - Reinstate Monica, May 02 '22 at 01:55

Clifford · Accepted Answer · 2022-05-02T17:38:25.613

It is somewhat more complex and inefficient than necessary, especially given the way strings work in C. You do not need to search for the newline. It is either at the end of the string, or it is not - the only place you need to look is at the end of the string.

fgets() can return NULL if for example EOF is encountered before any characters are read - this can easily happen if using input redirection from a file for example. So you should check for that. Your code will fail in unpredictable ways as-is if buff[0] != '\0' on entry and fgets() returns NULL.

It is normally useful to return something - even if often you discard the return value. You could return an error indication or return a valid (but empty) string on error, or possibly return the length of the input string to save the caller yet another strlen() traversal. Personally I'd suggest returning a pointer to the caller's buffer and initialising buff[0] = '\0' to ensure a valid string is always returned. That way you can do things such as:

char inp[20] ;
printf( "%s\n", get_input( inp, sizeof(inp) ) ) ;

So I would suggest:

char* get_input( char* buff, size_t size )
{
    buff[0] = '\0' ;
    
    if( fgets(buff, (int)size, stdin) != NULL )
    {
        size_t len = strlen( buff ) ;
        char* endp = len == 0 ? buff : &buff[len - 1] ;
        
        if(  *endp != '\n' )
        {
            int discard ;
            while( (discard = getchar()) != '\n' && discard != EOF ) ;
        }
        else
        {
            // Remove newline from buff
            *endp = '\0';
        }
    }
    
    return buff ;
}

One possibly useful modification, since you go to the trouble of determining the length of the input would be to return that to the caller via a reference parameter:

// Get input from user
char* get_input( char *buff, size_t size, size_t* inplen )
{
    buff[0] = '\0' ;
    size_t len = 0 ;
    
    if( fgets(buff, (int)size, stdin) != NULL )
    {
        len = strlen( buff ) ;
        char* endp = len == 0 ? buff : &buff[len - 1] ;
        
        if(  *endp != '\n' )
        {
            int discard ;
            while( (discard = getchar()) != '\n' && discard != EOF ) ;
        }
        else
        {
            // Remove newline from buff
            *endp = '\0';
            len-- ;
        }
    }
    
    // Return string length via inplen if provided
    if( inplen != NULL ) *inplen = len ;
    
    return buff ;
}

Then for example:

char inp[20] ;
size_t length = 0 ;
printf( "%s\n", get_input( inp, sizeof(inp), &length ) ) ;
printf( "%zu characters entered\n", length ) ;

Or if you want to discard the length:

get_input( inp, sizeof(inp), NULL ) ;

Thanks for the awesome recommendations. It's indeed way smarter to write it like that! — danilo, May 01 '22 at 21:45
Corner issues: Reading `"\n"` or just `EOF` both return `buff`. This is unlike `fgets()` that returns just encountering end-of-file or an input error, which return `NULL`. Caller has no info to certainly know if an excessively long line was read. `buff[strlen( buff ) - 1]` is UB if the first character read was a _null character_. `buff[0]` is UB if `size == 0`. — chux - Reinstate Monica, May 02 '22 at 01:38
@chux-ReinstateMonica : The point about the UB is accepted - will fix that. The semantics of the function are for the OP to decide. You could count the number of discarded characters and return the number of characters buffered and the number actually entered by a reference parameter. An exercise for the reader. I considered the issue of `size == 0 || buff = NULL`, but decided to consider valid input a prerequisite. Since `size == 0 || buff == NULL` is not a useful call an `assert()` might be in order. — Clifford, May 02 '22 at 17:00
I see `size == 0 || buff == NULL` as useful, like in `snprintf(NULL, 0, ...`. Here it would read a complete _line_, toss all of it and effectively return a yes, something was read or no, end-of-file immediately occurred. — chux - Reinstate Monica, May 02 '22 at 17:10
@chux-ReinstateMonica : fair point - perhaps beyond the scope of this question as it would implement semantics not suggested by the OPs original code. I'll leave the code as-is; anything else is for a different question perhaps, and your comments stand as notice to any reader of how this might be extended. — Clifford, May 02 '22 at 17:16
Why will my code fail is buff[0] is not '\0'? Why do I need to set buff[0] to '\0' at the beginning of the function? — danilo, May 14 '22 at 00:32
@danilo because `strlen(buff)` will return an unpredictable value. My version returns `buff`, if you do not terminate the string and `fgets()` fails, `get_input()` will return an unmodified buffer which may be an invalid string, with no indication there was an error - the caller using such a string will fail in ways that are hard to handle. The caller could of course have initialised the string, but this ray you need not rely on that. Returning NULL is an option, but you could not then reliably use the function call as an unchecked parameter as in the examples. — Clifford, May 14 '22 at 08:01

chux - Reinstate Monica · Answer 2 · 2022-05-02T01:56:50.600

.... it fail in some occasion?

Yes.

Missing check of return value from fgets(). fgets() returns NULL to indicate an immediate end-of-file or input error just occurred, so should get_input(). In such cases for OP's code, buff[] is in an indeterminate state as so strcspn(buff, "\n"); risks undefined behavior (UB).
Code fails to return any indication of success or error (end-of-file, input error that occurred here or excessive long line.).
Extreme buffer sizes may exceed the int range. size_t size will not exceed an array or allocation size. strcspn(buff, "\n") returns a size_t, not int. Saving in an int will cause issues if the value was more than INT_MAX.
strcspn(buff, "\n") fails to detect a '\n' if a prior null character was read.

Standard C lacks a robust way to read a line into a string. fgets() gets us mostly there.

score 0 · Answer 3 · answered May 01 '22 at 18:26

0

Yes, your code is ok, and it should not fail. However always check for fgets fail: (from manual)

If an error occurs, they return NULL and the buffer contents are indeterminate.

answered May 01 '22 at 18:26

Jakub Bednarski

261
3
9

Thanks for the heads up. I'll do this from now on – danilo May 01 '22 at 19:30

Is my code okay or will it fail in some occasion?

3 Answers3