Stack smashing detected in C - why does this happen?

Question

I have the following function, which, given a string, should find the most recurrent couple of letters in it and store the result in a different string. For example - for the string "ababa", the most recurrent couple would be "ba", and for "excxexd" it would be "ex". This is the code:

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

void printError(){
    printf("Error: please check your input\n");
}

bool isLexicographicallyPreceding(char couple1[], char couple2[])
{
    if (strcmp(couple1, couple2)>=0) return true;
    return false;
}

void coupleDetector(int length, char word[], char result[])
{
    char couples[length-1][2];
    for (int i=0; i<length-1; i++)
    {
        char couple[2] = {word[i], word[i+1]};
        strcpy(couples[i], couple);
    }
    char element[]="";
    int count=0;
    for (int j=0; j<length-1; j++)
    {
        char tempElement[2];
        strcpy(tempElement,couples[j]);
        int tempCount=0;
        for (int p=0; p<length-1; p++)
        {
            if (couples[p]==tempElement) tempCount++;
        }
         if (tempCount>count)
            {
                strcpy(element, tempElement);
                count=tempCount;
            }
            if (tempCount==count)
            {
                if (isLexicographicallyPreceding(tempElement,element) == true) strcpy(element, tempElement);
            }
    }
    strcpy(result,element);
}

int main() {
    //Supposed to print "ba" but instead presents "stack smashing detected".
    int length=5;
    char arr[] = "ababa";
    char mostCommonCouple[2];
    coupleDetector(length,arr,mostCommonCouple);
printf("%s", mostCommonCouple);
    return 0;
}

The code compiles without errors, but for some reason does not work as intended but prints out "stack smashing detected". Why would that be? Advices would be very helpful. Thanks.

Answer 1

In trying out your program, I found a few of your character arrays undersized. Character arrays (strings) need to be sized large enough to also include the null terminator value in the array. So in many locations, having a two-character array size is not sufficient and was the cause of the stack smashing. With that in mind, following is a refactored version of your program.

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

void printError()
{
    printf("Error: please check your input\n");
}

bool isLexicographicallyPreceding(char couple1[], char couple2[])
{
    if (strcmp(couple1, couple2)>=0) return true;
    return false;
}

void coupleDetector(int length, char word[], char result[])
{
    char couples[length-1][3];
    for (int i=0; i<length-1; i++)
    {
        char couple[3] = {word[i], word[i+1], '\0'};
        strcpy(couples[i], couple);
    }
    char element[3];        /* Define the character array */
    strcpy(element, "");    /* Then initialize it if need be */
    int count=0;
    for (int j=0; j<length-1; j++)
    {
        char tempElement[3];
        strcpy(tempElement,couples[j]);
        int tempCount=0;
        for (int p=0; p<length-1; p++)
        {
            if (couples[p]==tempElement) tempCount++;
        }
        if (tempCount>count)
        {
            strcpy(element, tempElement);
            count=tempCount;
        }
        if (tempCount==count)
        {
            if (isLexicographicallyPreceding(tempElement,element)) strcpy(element, tempElement);
        }
    }
    strcpy(result,element);
}

int main()
{
    //Supposed to print "ba" but instead presents "stack smashing detected".
    int length=5;
    char arr[] = "ababa";
    char mostCommonCouple[3];                   /* Notice size requirement to also contain the '\0' terminator */
    coupleDetector(length,arr,mostCommonCouple);
    printf("%s\n", mostCommonCouple);
    return 0;
}

Here are some key points.

• Viewing the code, most sizes for arrays was enlarged by one to accommodate storage of the null terminator.
• Work fields such as "element" need to be defined to their proper size so that subsequent usage won't also result in stack smashing.

Testing out the refactored code resulted in the following terminal output.

@Vera:~/C_Programs/Console/Recurrent/bin/Release$ ./Recurrent 
ba

So to reiterate, be cognizant that character arrays normally need to be defined to be large enough to contain the largest expected string plus one for the null terminator.

Give that a try and see if it meets the spirit of your project.