Why does referencing this char array cause Stack smashing, using C?

Question

The program takes a pointer to a char array and an int. The char array consists of two numbers, separated by a space.

The use of the function is to read the values of the char array as integers and replace them with the multiplied value of the input:

void read_and_mul(char *arr, int scale) {
    int num_arr[2];                         // saving values in a int[]
    char *ch = strtok(arr, " ");
    num_arr[0] = scale * (atoi(ch));
    ch = strtok(NULL, " ");
    num_arr[1] = scale * (atoi(ch));

    memset(arr, 0, sizeof(arr));      // deleting the previous value of the char[]

    char one[sizeof(int)];
    char two[sizeof(int)];
    sprintf(one, "%d", num_arr[0]);   // saving the altered numbers as chars
    sprintf(two, "%d", num_arr[1]);

    strcat(arr, one);                // writing the multiplied values to the string
    strcat(arr, " ");
    strcat(arr, two);
}

However if I use it like this, it works as intended but causes a stack-smashing:

int main(int argc, char *argv[]) {
    char str[] = "1 2";
    read_and_mul((char *)&str, 10);
    printf("string after call: %s\n", str);
    return 0;
}

The terminal message in CLion is:

*** stack smashing detected ***: terminated
string after call: 10 20

Is this a potential error or an IDE warning and what is causing it?

This character array char str[] = "1 2"; the size of which is equal to 4 is not large enough to store the string "10 20" the size of which is equal to 6. — Vlad from Moscow, Aug 10 '23 at 15:53
Pay attention to that using the expression sizeof( arr ) in this call of memset memset(arr,0,sizeof(arr)); does not make sense because arr is a pointer, So if sizeof( arr ) is equal to 8 then again there is an access beyond teh array. — Vlad from Moscow, Aug 10 '23 at 15:56
`char str[32] = "1 2";` leaves you some extra room to concatenate to the string, otherwise the array is only long enough for that string and you can't add anything to the end without going out of bounds. `char one[sizeof (int)];` is potentially too small as well unless your numbers are never more than 3 digits. — Retired Ninja, Aug 10 '23 at 15:56
@RetiredNinja: no, the numbers may be greater than 3 digits unfortunately. Is the wrong size of the new values the only issue or is there something wrong with my use of pointers? — HeapUnderStop, Aug 10 '23 at 15:59
@HeapUnderStop The issue you're having is mostly due to array sizes being too small which is easy to fix, just make them larger. `char one[16];` is even easier to type than what you had before. You should also always pass the length of an array to a function that uses it so you can use `memset` and such correctly. — Retired Ninja, Aug 10 '23 at 16:29

Vlad from Moscow · Accepted Answer · 2023-08-10T19:30:12.947

The function has to build the string "10 20" that contains 6 characters including the terminating null character '\0'.

But you are trying to store this string in an array that has only 4 characters

char str[] = "1 2";

due to these statements

strcat(arr,one);                // writing the multiplied values to the string
strcat(arr, " ");
strcat(arr,two);

As a result the function already invokes undefined behavior.

Another problem is in this call of memset:

memset(arr,0,sizeof(arr));

The variable arr within the function has the pointer type char *. If sizeof( char * ) is equal to 8 then again there is an attempt to write to memory outside the array.

And the function should not depend on magic numbers like 2 used in this declaration

int num_arr[2];

You should always try to write more general functions.

To resolve the problem you should within the function allocate dynamically a new character array where the result string will be stored and return a pointer to the array from the function.

Also, pay attention to that it will be more clear and correct to write

read_and_mul( str, 10 );

instead of

read_and_mul((char *) &str, 10);

Here is a demonstration program that shows a possible approach to solve the task.

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

char * read_and_mul( const char *s, int scale )
{
    size_t n = 0;
    size_t length = 0;

    const char *tmp = s;
    int value;

    for (char *endptr; value = strtol( tmp, &endptr, 10 ), endptr != tmp; tmp = endptr)
    {
        ++n;
        length += snprintf( NULL, 0, "%d", value * scale );
    }

    length += n == 0 ? 1 : n;

    char *result = calloc( length, sizeof( char ) );

    if (result != NULL)
    {
        const char *tmp = s;
        int first = 1;

        for (char *pos = result, *endptr; value = strtol( tmp, &endptr, 10 ), endptr != tmp; tmp = endptr)
        {
            if (!first)
            {
                *pos++ = ' ';
            }
            else
            {
                first = 0;
            }

            pos += sprintf( pos, "%d", value * scale );
        }
    }

    return result;
}

int main( void )
{
    char s[] = "1 2 3 4 5 6 7 8 9 10";

    char *result = read_and_mul( s, 10 );

    if (result) printf( "\"%s\"\n", result);

    free( result );
}

The program output is

"10 20 30 40 50 60 70 80 90 100"

As in general multiplication of two integers can result in overflow then to avoid such a situation you may change these statements

length += snprintf( NULL, 0, "%d", value * scale );
pos += sprintf( pos, "%d", value * scale );

to the following

length += snprintf( NULL, 0, "%lld", ( long long int )value * scale );
pos += sprintf( pos, "%lld", ( long long int )value * scale );

chqrlie · Answer 2 · 2023-08-10T21:27:31.680

There are multiple problems in the code:

char *ch = strtok(arr, " "); you do not check for strtok failure to find a word. strtok() would return a null pointer in this case and atoi(ch) would cause undefined behavior, probably a segmentation fault.
same problem for the second word
memset(arr, 0, sizeof(arr)); this is useless and sizeof(arr) is the size of a pointer, not the length of the array pointed to by arr. Another instance of undefined behavior if the array passed to the function is too small.
char one[sizeof(int)]; the size of type int in bytes is not the length of the representation of an integer in decimal digits. It is always too small. You do not need these intermediary arrays: you could compose the final string directly into the array pointed to by str.
sprintf(one, "%d", num_arr[0]); you cannot pass the size of the destination array to sprintf: you would have undefined behavior if the representation as a string does not fit in the target array. You should use snprintf instead.
strcat(arr, one); instead of relying on arr having been erased, you could use strcpy. But in all cases you assume that the target array is large enough for the constructed string: this is risky. It would be better for the function to take the array length as an extra argument and to protect against buffer overflow with snprintf().
char str[] = "1 2"; The array is just long enough for "1 2", it will be too short for "10 20". This is the stack smashing detected by your IDE.
read_and_mul((char *)&str, 10); instead of casting (char *)&str, you should just pass str as an argument: an array automatically decays into a pointer to its first element in this case.

Here is a modified version:

#include <stdio.h>

int read_and_mul(char *s, size_t size, int scale) {
    int a, b;
    if (sscanf(s, "%d%d", &a, &b) == 2) {
        // the string contains 2 integers,
        // attempt to overwrite with the scaled values
        size_t len = snprintf(s, size, "%d %d", a * scale, b * scale);
        if (len >= size)
            return -2; // overflow
        else
            return 0;
    }
    return -1; // parsing error: the string does not contain at least 2 integers
}

int main(int argc, char *argv[]) {
    char str[100] = "1 2";
    int status = read_and_mul(str, sizeof str, 10);
    printf("string after call: %s, status = %d\n", str, status);
    return 0;
}

Why does referencing this char array cause Stack smashing, using C?

2 Answers2