want to call fgets() multiple times based on users need

Question

User enters '1' or '0' choice to continue getting string using fgets(). So when the user enters the choice, fgets reads it from console. I am storing it in another variable. But fgets gets the choice and stores it in messages. I have tried using fflush(stdin) after receiving the choice. Please help me out.

int main() {
    int choice=1;
    char *message;
    int i=0;
    while (choice == 1) {
        fflush(stdout);
        printf("Enter the message: ");
        fflush(stdout);
        message = fgets(message,200,stdin);
        while (message[i]!='\n') {
            i++;
        }
        message[i] = '\0';
        send_message(message);
        printf("\nType '1' to continue or '0' to quit: ");
        scanf("%d",&choice);
        fflush(stdin);
     }
 }

`fflush(stdin);` is undefined behavior. Why not just read the newline and then go back to reading a line at a time? Alternatively, just use fgets for everything and parse the int. https://stackoverflow.com/questions/2979209/using-fflushstdin — Retired Ninja, Aug 03 '17 at 01:36
`char *message; ... fgets(message,200,stdin);` passes an uninitialized `message` to `fgets()`. What do you expect a function to do with such data? — chux - Reinstate Monica, Aug 03 '17 at 02:00
Telling the user to type `y` or `n` and then trying to read an integer with `"%d"` is not going to lead to happiness. Your users will be confused, even if you are not. Another of your main problems looks to be that [`scanf()` leaves newlines behind](https://stackoverflow.com/questions/5240789/scanf-leaves-the-new-line-char-in-buffer). — Jonathan Leffler, Aug 03 '17 at 03:10

Steven K. Mariner · Answer 1 · 2017-08-03T02:18:23.887

2

It looks like you're trying to scanf() to read the user's input -- this is inherently dangerous. (See https://www.reddit.com/r/learnprogramming/comments/1d0w4x/c_scanf_d_but_error_if_user_enters_a_character/).

I'd recommend either using %s for your format string, or better yet, build a subroutine to do safe input and parse it the old-fashioned way, such as something along these lines:

/* getsafe() - Generic input using the preferred input method rather than gets() */

#include <stdio.h>
#include <string.h>


char *getsafe(char *inpstr,int inpsiz) {
    char    *seachr;                    /* Result of search via strchr()     */

    if (inpstr==NULL) {
        return(NULL);
    }
    if (fgets(inpstr,inpsiz,stdin)==NULL) {
        return(NULL);
    }
    seachr=strchr(inpstr,'\n');
    if (seachr!=NULL) *seachr=0;

    return(inpstr);
}

That way you can specify the buffer length and provide a string (array of characters) of sufficient length as to prevent buffer overruns (security issue), and then parse the [0] position in that array for your answer.

#define ANSSIZ 80               /* Maximum allowed size of user answer    */
char usrans[ANSSIZ];            /* User Answer                            */
printf("Enter 'y' or 'n': ");
getsafe(usrans, ANSSIZ-1);

edited Aug 03 '17 at 02:18

answered Aug 03 '17 at 01:44

Steven K. Mariner

441
3
9

Added as requested. ++chux Also, I've been know to throw unreasonably large buffers at `getsafe()` (as in `#define ANSSIZ 10240`. Got laughed at by a very young programmer for using such large buffers and worrying about buffer overruns -- and code he released to FidoNet became an attack vector LOL. – Steven K. Mariner Aug 03 '17 at 02:19
1

The minus 1 in `ANSSIZ-1` not needed. – chux - Reinstate Monica Aug 03 '17 at 02:21
I'll take you at your word, but would verify with fencepost testing before I ceased the practice. It's been a long, long, very long time since I wrote that subroutine. – Steven K. Mariner Aug 03 '17 at 02:27
1

Perhasp "The `fgets` function reads at most one less than the number of characters specified by `n` ..." C11dr §7.21.7.2 2 may help. – chux - Reinstate Monica Aug 03 '17 at 02:32
1

I like the `#define ANSSIZ 10240` story. IMO, user input is _evil_ and not to be trusted until vetted. – chux - Reinstate Monica Aug 03 '17 at 02:34
*...for using such large buffers and worrying about buffer overruns...* -- The solution for buffer over runs in not throwing large buffers but adding proper limits while checking. Creating such large buffers in the auto scope leads to severe performance penalty and doesn't protect anyway. I am sure the same code he released would also be vulnerable even with `#define ANSSIZ 10240`, just slower. – Ajay Brahmakshatriya Aug 03 '17 at 03:27
Yes, `getsafe()` enforces the size limits. The large buffer was generally to ensure that no normal user would lose data, but the young programmer was unable to discern both the difference in functionality as well as the risk. Still makes for a funny story. – Steven K. Mariner Aug 03 '17 at 03:32

Addison · Answer 2 · 2017-08-03T02:19:32.560

There's a lot of problems with this - It probably belongs on Code Review

However, here is a critique on some of the major problems

int main() {
    int choice=1;
    char *message; // This is a pointer, but is not malloc'ed. You might want "char message[200]" instead?
    int i=0; // This is the only time "i" is set to 0. It needs to be reset at the start of the loop
    while (choice == 1) {
        fflush(stdout); // No need for this
        printf("Enter the message: ");
        fflush(stdout);
        message = fgets(message,200,stdin);
        while (message[i]!='\n') { // Why not use strlen?
            i++; // "i" can keep growing forever if there is no newline (if someone entered 199 characters before pressing enter)
        }
        message[i] = '\0'; // fgets does this for you - The past loop was pointless
        send_message(message);
        printf("\nType 'y' to continue or 'n' to quit: "); // You forgot to flush here!
        scanf("%d",&choice); // I don't think this will result in a 0 or 1 output... %d is for a digit, and you're asking the user for y or n.
        fflush(stdin); // This is invalid and unneeded - You can't flush stdin
    }
}

want to call fgets() multiple times based on users need

2 Answers2