Stack overflow correction in C++

Question

I read the code below in a book which said that this was vulnerable to stack overflow. Although fgets() has been used,I was not able to understand, why it is vulnerable?

My understanding is that using fgets() instead of gets() usually helps us get rid of buffer overflow by placing a null at the end. Am I missing something? What should be used instead of fgets() to correct the stack overflow?

void getinp(char *inp, int siz)
{
   puts("Input value: ");
   fgets(inp, siz, stdin);
   printf("buffer3 getinp read %s\n", inp);
}

void display(char * val)
{
   char tmp[16];
   sprintf(tmp, "read val: %s\n", val);
   puts(tmp);
}

int main(int argc, char *argv[])
{
   char buf[16];
   getinp(buf, sizeof(buf));
   display(buf);
   printf("buffer3 done\n");
}

Looking at the code, as well as how the two answers interpret it, I assume this isn't actually about a stack overflow, but rather an overflow of the 16-byte buffer you use to read text. That buffer is in fact allocated on the stack, but even if there is an overflow in that buffer, I wouldn't call that a stack overflow. — jogojapan
– jogojapan, Commented Nov 5, 2012 at 1:51
Do not confuse a stack overflow (which typically occurs by infinite recursion) with a buffer overflow, which is writing too much data into a buffer; in particular, a stack buffer overflow is an overflow of a buffer stored on the stack. — Adam Rosenfield
– Adam Rosenfield, Commented Nov 5, 2012 at 2:05

Matteo Italia · Accepted Answer · 2012-11-05 01:51:22Z

1

In display tmp is declared as 16 chars long, but you are writing (with the sprintf) there not only val (which is guaranteed to be 16 characters or less), but also "read val: " and the final \n).

This means that if the user inserts more than 16-11=5 characters you have a buffer overflow in display.

One solution could be declaring buf in display to be large enough to store both val and the additional text, although in the real world you would just write to stdout using printf (without the intermediate buffer).

Also, usually when you have a sprintf and there's some potential risk of buffer overflow you use snprintf instead (actually, I use it always); snprintf, instead of overflowing the buffer, truncates the output if it would be too long, and returns the number of characters that would have been written if the output buffer was big enough.

edited Nov 5, 2012 at 1:51

answered Nov 5, 2012 at 1:43

Matteo Italia

128k18 gold badges219 silver badges313 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

user1798982 Over a year ago

Thank you for the quick replies. So, the function getinp is not responsible for the overflow, it is the inbuilt sprintf function that is doing this.

user1798982 Over a year ago

Will adding this line to the above program in the function void display correct the buffer overflow?? snprintf(tmp, sizeof(tmp), "read val: %s\n", val);

Matteo Italia Over a year ago

That will prevent the buffer overflow, but won't allow input strings longer than 5 characters to be printed. You should also enlarge buf (or simply use printf instead of sprintf+puts).

James Harris · Accepted Answer · 2012-11-05 01:42:44Z

0

In display, there is no way to be sure that val + 12 bytes is going fit into a 16 character buffer.

answered Nov 5, 2012 at 1:42

James Harris

1,91411 silver badges15 bronze badges

Collectives™ on Stack Overflow

Stack overflow correction in C++

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related