Why do I get a segmentation fault by declaring a 2d array in c?

Question

I am new to threads and I have a program that uses threads to find the minimum number out of a 2d array and later on, it finds the distance that the other elements of the array have from the minimum number and stores them in another array.

The user should enter the size of the array and the number of threads he wants to use.

I tried the program below for 1d array and it worked just fine. When I converted it to work for a 2d array it started crashing and throwing a segmentation fault. I, however, cannot find which part of the 2d declaration is wrong.

Any help is really appreciated.

Here is my code:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>
#include <string.h>
#include <pthread.h>

struct Parameters
{
    // input
    int s,p; //n is size of array, p is number of threads
    int** array; //array with elements
    int start;
    int end;

    // output
    int smallest;
    int pos; //position if minimum
    int** B; //array that holds the distances
};

void* min(void* args)
{
    struct Parameters* p = (struct Parameters*)args;
    int **array = p->array;
    int **B1 = p->B;
    int start = p->start;
    int end = p->end;
    int smallest = array[start][start];
    int pos = p->pos;
    int distance;

    //find the smallest
    for (int i = start; i < end; i++)
    {
        for(int j = start; j < end; j++)
        {
            if (array[i][j] < smallest)
            {
                smallest = array[i][j];
                pos = i;
            }
        }  
    }

    //find the distances
    for(int i = 0; i < ((struct Parameters*)args) -> s; i++)
    {
        for(int j = 0; j < ((struct Parameters*)args) -> s; j++)
        {
            distance = abs(pos - i);
            B1[i][j] = distance;
        }
    }


    params->smallest = smallest;
    params->B = B1;

    return NULL;
}

int main()
{
    int smallest,pos;
    int s,p;

    struct Parameters *ptr = (struct Parameters *)malloc(sizeof(struct Parameters));

    if(ptr == NULL)
    {
        printf("Not enough. Try again \n");
        exit(0);
    }

    printf("Type s\n");
    scanf("%d",&(ptr->s));


    printf("Type p\n");
    scanf("%d", &(ptr->p));

    // declare an array of threads and associated parameter instances
    pthread_t threads[(ptr->p)];
    struct Parameters thread_parameters[(ptr->p)] ;

    int arr[ptr->s][ptr->s];
    int B2[ptr->s][ptr->s];

    // intialize the array    
    for(int i=0; i< ptr->s; i++)
    {
        for(int j=0; j< ptr->s; j++)
        {
        printf("Type a \n");
        scanf("%d",&arr[i][j]);
        }
    }

    // smallest needs to be set to something
    smallest = arr[0][0];

    // start all the threads
    for (int i = 0; i < ptr->p; i++)
    {
        memcpy(arr, thread_parameters[i].array, sizeof(arr));
        thread_parameters[i].s = ptr->s;
        memcpy(Bb, thread_parameters[i].B, sizeof(B2));
        thread_parameters[i].start = i * (ptr->s / ptr->p);
        thread_parameters[i].end = (i+1) * (ptr->s / ptr->p);
        pthread_create(&threads[i], NULL, min, &thread_parameters[i]);
    }

    // wait for all the threads to complete
    for (int i = 0; i < ptr->p; i++)
    {
        pthread_join(threads[i], NULL);
    }

    // Now aggregate the "smallest" and "largest" results from all thread runs    
    for (int i = 0; i < ptr->p; i++)
    {
        if (thread_parameters[i].smallest < smallest)
        {
            smallest = thread_parameters[i].smallest;
        }
    }

    printf("Smallest is %d\n", smallest);

    thread_parameters[ptr->p].B[ptr->s][ptr->s];

    for (int i = 0; i < 1; i++)
    {
        for(int j = 0; j < ptr->s;j++)
        {
            for(int k = 0; k < ptr->s; k++)
            {
                printf("Element %d is %d away from min\n",j,thread_parameters[i].B[j][k]);
            }
        }
   }

    return 0;
}

Thank you!!

Answer 1

The issue with your code might also come from :

memcpy(arr, thread_parameters[i].array, sizeof(arr));
...
memcpy(Bb, thread_parameters[i].B, sizeof(B2));

as thread_parameters[i].array and thread_parameters[i].B are not allocated, if you are only reading the array it might b fine to only pass them by address

thread_parameters[i].array = arr

but for thread_parameters[i].B you would need to allocate the arrays and perform a deep copy (memcpy would not work)

---

The below text does not answer the question but does provide some insight on VLA usage

One reason for causing the segmentation with a declaration of a Variable Length Array is that the value is to large to allocate the array on the stack (some compiler choose this option, this choice might have performance reason).
The is not much option to recover cleanly from failure to allocate memory on the stack as there is little way to clean up stack memory during runtime within the same stack context.

You can mitigate the issue by allocating your 2D arrays on the heap instead, some of the strategies are available here(thanks @Lundin) and here.

int** alloc_2d_int_array(size_t rows, size_t cols) {
     int **result = malloc(rows * sizeof(int *));
     if(result == NULL) {
          // could not allocate more memory
          return NULL;
     }
     size_t row_size = cols * sizeof(int); 
     for(int i=0; i < rows; ++i) {
         result[i] = malloc(row_size);
         if(result[i] == NULL) {
              // could not allocate more memory
              // cleanup
              return NULL;
         }
     }
     return result;
}

the above implementation have not been tested, but does compile, there are still risk of integer overflow.

Then use the above define function as following:

int **arr = alloc_2d_int_array(ptr->s, ptr->s);
int **B2 = alloc_2d_int_array(ptr->s, ptr->s);

easier implementation (see here(thanks @Lundin))

int **arr = malloc(sizeof(int[ptr->s][ptr->s]);
int **B2 = malloc(sizeof(int[ptr->s][ptr->s]);