0

Environment: VS2013 express, Windows 7.

Source codes are really simple:

#include <stdio.h>
#include <stdlib.h>
int main()
{
    int TestNum, k, idx;
    char *strbuf1 = NULL;
    strbuf1 = (char *)malloc(sizeof(char) * 10001);
    if (strbuf1 == NULL){
        printf("memory allocation failed\n");
        return -1;
    }

    gets(strbuf1);
    TestNum = atoi(strbuf1);
    for (k = 0; k < TestNum; k++){
        gets(strbuf1);
        printf("k= %d, strbuf1=%s\n", k, strbuf1);
        //--- read data ---//
        idx = 0;
        while (idx < 5){
            gets(strbuf1);
            idx ++;
        }
    }
    return 0;
}

After building the codes into an executable file, say foo.exe, I tested it with "foo.exe < testinput.txt" under cmd window. It will break down all the way, but I can't tell why. Anybody has a clue?

I've uploaded the "testinput.txt" file onto the GDrive, https://docs.google.com/document/d/1d8jBPZfYYjtA9R1CldUZhyRvaAiK5Xk9K-mhE6dIDKU/edit?usp=sharing

Jedi
  • 882
  • 1
  • 8
  • 24

1 Answers1

2

Replace this line:

gets(strbuf1);

with:

fgets(strbuf1, 10000, stdin);

This is because fgets has parameter for buffer size to avoid overflow, which gets does not have and thus prone to buffer overflows.

mvp
  • 111,019
  • 13
  • 122
  • 148
  • 1
    You mean `10001` (the size of `strbuf1`)? – David Ranieri Aug 22 '14 at 09:08
  • I used one char less to account for last 0 that should finish a string. It makes sense to set that last char to 0 just in case. – mvp Aug 22 '14 at 09:10
  • 1
    from [http://linux.die.net/man/3/fgets]: fgets() reads at most one less than size characters. Thus, 10001 was the correct value – Ingo Leonhardt Aug 22 '14 at 09:14
  • 2
    The `fgets()` function shall read bytes from stream into the array pointed to by s, **until n-1 bytes** are read, or a is read and transferred to s. Seems that OP wants 10000 chars (not 9999) – David Ranieri Aug 22 '14 at 09:14
  • 1
    fgets() reads in at most one less than size characters from stream and stores them into the buffer pointed to by s, which means in this case it has to be fgets(strbuf1, 10001, stdin) – Santosh A Aug 22 '14 at 09:18
  • Ok, ok, I am over-protective! It is still good idea to set last byte of any large array to 0 - some string functions do not set last byte to 0, and this may lead to overflows again. I think spending 1 extra byte for this is worth it – mvp Aug 22 '14 at 09:18
  • 1
    "*I think spending 1 extra byte for this is worth it*" no, its useless and makes the code more difficult to understand, as it's logically inconsistent. If you do so, at least it shall explicitly be documented, that your are wasting memory by intention. – alk Aug 22 '14 at 09:26
  • Well, the whole point of this question was OP not understanding what is buffer overflow. What I tried to do here is to be super-protective and make sure that OP does not encounter another buffer overflow if he starts using functions like **[`strncpy`](http://linux.die.net/man/3/strncpy)**, which are notorious for not terminating last character with 0 if buffer is full – mvp Aug 22 '14 at 09:33
  • 1
    `fgets()` **always** returns a `0`-terminated string (if not failing). There definitly is **no** need to provide an Angst-Byte. – alk Aug 22 '14 at 09:34
  • Sure, `fgets()` does, but `strncpy` (and maybe some other string functions I missed) does not. I am suggesting to waste single byte for this to avoid surprises forever. If I wrote my own code and know exactly what's going on, I probably wouldn't - because I knew better. – mvp Aug 22 '14 at 09:38