Stack smashing detected

Question

#include <iostream>
using namespace std;
int main()
{
    int tablica[9];
    string inputromanum;
    cout << "ROMAN: ";
    cin >> inputromanum;
    int maxindeks;
    bool disablenextcomp = false;
    int readysolution = 0;
    maxindeks = inputromanum.length() - 1;{}{}
    for (int i = 0; i <= maxindeks; i++)
    {
        if (inputromanum[i] == 'M' || inputromanum[i] == 'm')
        {
            tablica[i] = 1000;
        }
        if (inputromanum[i] == 'D' || inputromanum[i] == 'd')
        {
            tablica[i] = 500;
        }
        if (inputromanum[i] == 'C'|| inputromanum[i] == 'c')
        {
            tablica[i] = 100;
        }
        if (inputromanum[i] == 'L' || inputromanum[i] == 'l')
        {
            tablica[i] = 50;
        }
        if (inputromanum[i] == 'X' || inputromanum[i] == 'x')
        {
            tablica[i] = 10;
        }
        if (inputromanum[i] == 'V' || inputromanum[i] == 'v')
        {
            tablica[i] = 5;
        }
        if (inputromanum[i] == 'I' || inputromanum[i] == 'i')
        {
            tablica[i] = 1;
        }
    }
    cout<<endl;
    for(int i4 = 0; i4 <= maxindeks; i4++)
    {
        cout<<"tablica["<<i4<<"] = "<<tablica[i4]<<endl;
    }
    for (int i2 = 0; i2 <= maxindeks; i2++)
    {
        int i5 = i2 + 1;
        if (i5 <= maxindeks)
        {
            //cout<<endl<<"tablica[i2 + 1] = "<<tablica[i2 + 1];
            //cout<<endl<<"tablica[i2] = "<<tablica[i2];
            //cout<<endl<<"tablica[i2 + 1] - tablica[i2] = "<<tablica[i2 + 1] - tablica[i2];
            if (tablica[i2 + 1] - tablica[i2] > 0 && disablenextcomp == false)
            { 
                //cout<<endl<<"readysolution + (tablica[i2 + 1] - tablica[i2]) = "<<readysolution + (tablica[i2 + 1] - tablica[i2])<<endl;
                readysolution = readysolution + (tablica[i2 + 1] - tablica[i2]);
                disablenextcomp = true;
            }
            else
            {
                if(disablenextcomp == false)
                {
                    //cout<<endl<<"readysolution + tablica[i2] = "<<readysolution + tablica[i2]<<endl;
                    readysolution = readysolution +  tablica[i2];
                }
                else
                {
                    disablenextcomp = false;
                }
            }
        }
        else
        {
            if(disablenextcomp == false)
            {
                //cout<<endl<<endl<<"OSTATNI INDEKS";
                //cout<<endl<<"tablica[i2] = "<<tablica[i2];
                //cout<<endl<<"readysolution + tablica[i2] = "<<readysolution + tablica[i2];
                readysolution = readysolution +  tablica[i2];
            }
        }
        i5++;
    }
    cout << endl << readysolution;
}

This is my program. made for decoding roman numerals into arabic ones. It works as intended in most cases, however, one of my colleagues found it to produce this error while inputting MMMCMXCVIII into the program:

*** stack smashing detected ***: terminated

It would refuse to work afterwards.

I wasn't able to find different numbers that would cause this error except MMMMMMMMMMM. It seems to fail when the index of tablica array exceeds 10. I don't know why it does so, as i am a novice in c++. It should've outputted 3999 instead of the error appearing. The numbers it should process successfully should range from 1 to 5000.

"Stack Smashing" means you've overwritten a buffer to the point where you've written beyond the stack frame. You code has a buffer overflow vulnerability. — h0r53, Dec 22 '22 at 16:40
If `maxindeks` is 9 or more, you go outside the bounds of `tablica` — ChrisMM, Dec 22 '22 at 16:40
To diagnose "stack smashing" and "heap corruption", add an `assert(i>=0 && i — Mooing Duck, Dec 22 '22 at 16:40
`inputromanum` is where you store unbounded user input. However, `tablica` is defined to only store `9` integers. If a user enters a string over 9 characters, you will experience a buffer overflow in your `for` loop that writes to `tablica[i]`, because the value of `i` is `9` or greater. — h0r53, Dec 22 '22 at 16:42
Offtopic: write code in English only. I see some symbols are using Polish words. Worst case where you mix English and Polish in single symbol (`maxindeks`). — Marek R, Dec 22 '22 at 16:50

score 0 · Answer 1 · answered Dec 22 '22 at 16:51

Thanks to folks in the comments, I've found the cause. The tablica[9] array is supposed to store 9 or less characters. The length of the input (MMMCMXCVIII in this case) has more characters, therefore it makes the for loop responsible for storing values for each character to cause mentioned above error, as there are no remaining units to store the values in. I've expanded the storage of tablica to 25 characters.

sklott · Answer 2 · 2022-12-22T18:10:22.830

In modern C++ it is considered bad practice to use C-style arrays and index loops whenever you can avoid this. So, fo example you can rewrite first loop like this:

    std::vector<int> tablica;
    tablica.reserve(inputromanum.size()); // This line is not necessary, but it can help optimize memory allocations
    for (char c : inputromanum)
    {
        if (c == 'M' || c == 'm')
        {
            tablica.push_back(1000);
        }
        if (c == 'D' || c == 'd')
        {
            tablica.push_back(500);
        }
        if (c == 'C'|| c == 'c')
        {
            tablica.push_back(100);
        }
        if (c == 'L' || c == 'l')
        {
            tablica.push_back(50);
        }
        if (c == 'X' || c == 'x')
        {
            tablica.push_back(10);
        }
        if (c == 'V' || c == 'v')
        {
            tablica.push_back(5);
        }
        if (c == 'I' || c == 'i')
        {
            tablica.push_back(1);
        }
    }

And you will avoid your issue completly. Something similar can be done with other loops too. This approach also has benefit of (somewhat) properly handling situations when input line has other symbols, which is not roman number. Try it on your version and you will see what I mean.

One more point. When you need to do something different depending of value of one variable, like you did with all those ifs. There is special statement in C/C++ for this: switch. So instead of those ifs you can do this:

    std::vector<int> tablica;
    tablica.reserve(inputromanum.size()); // This line is not necessary, but it can help optimize memory allocations
    for (char c : inputromanum)
    {
        switch(c)
        {
            case 'M':
            case 'm':
                tablica.push_back(1000);
                break;
            case 'D':
            case 'd':
                tablica.push_back(500);
                break;
            case 'C':
            case 'c':
                tablica.push_back(100);
                break;
            case 'L':
            case 'l':
                tablica.push_back(50);
                break;
            case 'X':
            case 'x':
                tablica.push_back(10);
                break;
            case 'V':
            case 'v':
                tablica.push_back(5);
                break;
            case 'I':
            case 'i':
                tablica.push_back(1);
                break;
        }
    }

Stack smashing detected

2 Answers2