Is this vulnerable to a stack overflow?

Question

void gctinp (char *inp, int siz)
{

  puts ("Input value: ");
  fgets (inp, siz, stdin);
  printf ("buffer3 getinp read %s", inp);
}

From what I've read, fgets is supposed to be used when you want to limit the size of input. So this code shouldn't be vulnerable right?

It is being called like so:

int main (int argc, char *argv[])

{

 char buf[16];

 getinp (buf, sizeof (buf));

 display (buf);

 printf ("buffer3 done\n");

}

Thanks for your time.

Answer 1

You won't strike buffer overflow problems if you enter more characters than can be safely stored since fgets restricts the input. It also adds a null terminator (assuming buffer size is greater than 0, of course).

However, you will have problems with information being left in the input buffer the next time you try to read something - this is something that users will find very annoying, entering something like hello again and having it treated as two separate inputs like hello ag and ain. And there's no indication given by fgets that it stopped retrieving input before the end of the line so, as far as your code is aware, everything is fine.

The major things you need to look out for (re buffer overflows on input) are, at a minimum, scanf with an unbounded %s format string and gets, which has no limiting size argument, neither of which are in your code.

If you're looking for a more robust input solution with size limiting, prompting and buffer clearing, check out this code, which provides all those features:

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

#define OK       0
#define NO_INPUT 1
#define TOO_LONG 2
static int getLine (char *prmpt, char *buff, size_t sz) {
    int ch, extra;

    // Get line with buffer overrun protection.
    if (prmpt != NULL) {
        printf ("%s", prmpt);
        fflush (stdout);
    }
    if (fgets (buff, sz, stdin) == NULL)
        return NO_INPUT;

    // If it was too long, there'll be no newline. In that case, we flush
    // to end of line so that excess doesn't affect the next call.
    if (buff[strlen(buff)-1] != '\n') {
        extra = 0;
        while (((ch = getchar()) != '\n') && (ch != EOF))
            extra = 1;
        return (extra == 1) ? TOO_LONG : OK;
    }

    // Otherwise remove newline and give string back to caller.
    buff[strlen(buff)-1] = '\0';
    return OK;
}

 

// Test program for getLine().

int main (void) {
    int rc;
    char buff[10];

    rc = getLine ("Enter string> ", buff, sizeof(buff));
    if (rc == NO_INPUT) {
        // Extra NL since my system doesn't output that on EOF.
        printf ("\nNo input\n");
        return 1;
    }

    if (rc == TOO_LONG) {
        printf ("Input too long [%s]\n", buff);
        rc = getLine ("Hit ENTER to check remains> ", buff, sizeof(buff));
        printf ("Excess [%s]\n", buff);
        return 1;
    }

    printf ("OK [%s]\n", buff);

    return 0;
}

And, doing some basic tests:

pax> ./prog
Enter string> [CTRL-D]
No input

pax> ./prog
Enter string> x
OK [x]

pax> ./prog
Enter string> hello
OK [hello]

pax> ./prog
Enter string> hello from earth
Input too long [hello fro]
Hit ENTER to check remains> [ENTER]
Excess []

pax> ./prog
Enter string> i am pax
OK [i am pax]

Answer 2

No, it isn't prone to stack overflow.

Are you confusing stack overflow and buffer overflow by any chance?

http://en.wikipedia.org/wiki/Stack_overflow

Answer 3

fgets will read at most one less than the specified number of bytes, and will make sure that the read string is null-terminated. So as long as you pass the correct size, it should be fine (although the string might not end in a newline).