C: Update shared memory pthreads and using strncpy-CodePudding

I'm trying to learn how to update some shared memory using threads without using global variables, but I keep getting "segmentation faults (core dumped)" errors when I try to use strncpy?

I am trying to reference the unique data structs from the shared memory in each thread?

Any hints would be appreciated!

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <time.h>
#include <pthread.h>
#include <semaphore.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/time.h>
#include <unistd.h>
#include <fcntl.h>
#include <math.h>
#include <sys/stat.h>
#include <sys/wait.h>
#include <string.h>

#define THREADCOUNT 3
#define ARRAYCOUNT 3
#define SHM_SIZE 48
#define KEY_NAME "test"

const int threadNumArray[3] = {0, 1, 2};

/* Data struct */
typedef struct struct_test
{
    int testi;     /* 4 bytes */
    float testf;   /* 4 bytes */
    char testc[6]; /* 8 bytes (2 bytes padding) */
} test_t;

/* Arrays of data */
typedef struct shared_array_test
{
    test_t test_array[ARRAYCOUNT];
} shm_array_t;

/* Pointer to arrays */
typedef struct shared_pointer_test
{
    shm_array_t *array_ptr;
} shm_pointer_t;

/* Thread structs with pointer to data array and thread number */
typedef struct thread_array_test
{
    int threadNum;
    shm_pointer_t *shared_ptr;
} thread_array_t;

void *test_process(void *arg)
{
    thread_array_t *thread_array = (void *)arg;                                                    /* Pointer to shared data */
    test_t *test_data = &thread_array->shared_ptr->array_ptr->test_array[thread_array->threadNum]; /* Data from thread number */
    char *testing = "TESTING";                                                                     /* String */
    strncpy(test_data->testc, testing, 6);                                                         /* String copy to shared segement */
    test_data->testf = 10.2;                                                                       /* Assign float */
    test_data->testi = 2;                                                                          /* Test integer */
    return NULL;
}

int main()
{
    /* Create shared memory segement */
    int shm_test;
    shm_pointer_t *shared_test;
    if ((shm_test = shm_open(KEY_NAME, O_CREAT | O_RDWR, 0666)) < 0)
    {
        printf("Error: shm_open");
        return -1;
    }
    ftruncate(shm_test, SHM_SIZE);
    if ((shared_test = mmap(0, SHM_SIZE, PROT_WRITE, MAP_SHARED, shm_test, 0)) == MAP_FAILED)
    {
        printf("Error: mmap");
        return -1;
    }

    /* Creates structs for each thread */
    thread_array_t thread_array[THREADCOUNT];
    for (int i = 0; i < THREADCOUNT; i  )
    {
        thread_array[i].shared_ptr = shared_test;      /* Shared data */
        thread_array[i].threadNum = threadNumArray[i]; /* Thread id */
    }

    /* Start threads */
    pthread_t threads[THREADCOUNT];
    for (int i = 0; i < THREADCOUNT; i  )
    {
        pthread_create(&threads[i], NULL, test_process, thread_array   i);
    }
    /* Join threads */
    for (int i = 0; i < THREADCOUNT; i  )
    {
        pthread_join(threads[i], NULL);
    }
    /* Test - Print strings */
    for (int i = 0; i < THREADCOUNT; i  )
    {
        printf("Test: %s", shared_test->array_ptr->test_array->testc);
    }
    return 0;
}

CodePudding user response：

Preliminarily, do note that it is altogether unnecessary to set up a POSIX shared memory segment to share data among threads of the same process. POSIX shared memory is intended primarily to allow separate processes to share memory. Threads of the same process operate in the same address space as each other, and they automatically have access to the contents of that space. This is, in fact, one of the key characteristics of threads.

With that out of the way, your code is unreasonably hard to follow on account of all the typedefing and unnecessary wrapper structures. Also, substantially all the names are very uninformative. With all that going on, it is not surprising that you have confused yourself.

Here:

    if ((shared_test = mmap(0, SHM_SIZE, PROT_WRITE, MAP_SHARED, shm_test, 0)) == MAP_FAILED)

... you assign a pointer to the shared memory segment to variable shared_test, of type shm_pointer_t *, a.k.a. struct shared_pointer_test *. That memory is not initialized, and in particular, shared_test->array_ptr is not assigned to point to anything.

You give all the threads copies of the shared_test pointer, and they attempt to use it as if the array_ptr member was a valid pointer. This is very likely the source of your issue, although there are other flaws that would likely have had similar observable effects (see below).

Additionally,

It does not help that you use a numeric constant for SHM_SIZE, as that provides no insight on how the size was chosen or what may be the intended use of the memory. Use sizeof instead of an integer constant. Also, because its use is so localized, I don't think making a macro of this is actually helpful -- the code would clearer without.
strncpy() does not add a null terminator if it doesn't encounter one within the number of characters specified by its third argument. In your case, this would result in your threads writing unterminated character arrays into the various testc members if the program actually reached that point. That is not inherently erroneous, but you cannot then treat those arrays as strings, which main() later attempts to do. strncat() is often a better choice than strncpy(), even though the former may require prepping the destination by writing a terminator to its first position.
you seem to be at least a little confused between POSIX shared memory (which you are using) and System V shared memory. Specifically, "KEY_NAME" is reminiscent of SysV IPC keys, whereas POSIX shared memory objects just have names. And those names should start with "/", which yours does not.
Good code comments are very valuable, but useless code comments are worse than no comment. Comments should explain things that are important but not obvious, such as the reason for an implementation choice, the expectations or preconditions for a function or piece of code, or similar.
Do not attempt to compute structure sizes manually. It is error-prone and non-portable. Instead, use sizeof to compute object and datatype sizes where needed.
main() should generally return a number in the range 0 - 125, not -1
perror can be a handy way to emit error messages about failed calls to library functions.
Although literal 0 is a valid null-pointer constant, it is better form to use NULL for the purpose, as this makes the intent more clear.
As a matter of code style, avoid including unneeded headers.

Here's a variation on your code that removes a lot of the goop, improves (IMO) much of the naming, and fixes most of the style issues. It is both shorter and clearer than the original, and it also happens to work correctly.

#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/types.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <fcntl.h>

#define THREADCOUNT 3
#define SHM_NAME "/test"

/* Data struct */
struct item {
    int testi;     /* 4 bytes */
    float testf;   /* 4 bytes */
    char testc[6]; /* 8 bytes (2 bytes padding) */
};

/* The structure of the shared memory contents */
struct shared_data {
    struct item test_array[THREADCOUNT];
};

/* Per-thread information */
struct thread_info {
    int threadNum;
    struct shared_data *shared_ptr;
};

void *test_process(void *arg) {
    struct thread_info *my_info = arg;
    struct item *test_data = &my_info->shared_ptr->test_array[my_info->threadNum];

    test_data->testc[0] = '\0';
    strncat(test_data->testc, "TESTING", sizeof(test_data->testc) - 1);
    test_data->testf = 10.2;
    test_data->testi = 2;

    return NULL;
}

int main(void) {
    /* Create shared memory segement */
    int shm_test;
    struct shared_data *shared;

    if ((shm_test = shm_open(SHM_NAME, O_CREAT | O_RDWR, 0666)) < 0) {
        perror("shm_open");
        return 1;
    }
    ftruncate(shm_test, sizeof(*shared));
    if ((shared = mmap(0, SHM_SIZE, PROT_WRITE, MAP_SHARED, shm_test, 0)) == MAP_FAILED) {
        perror("mmap");
        return 1;
    }

    /* Set up per-thread data */
    struct thread_info thread_array[THREADCOUNT];
    for (int i = 0; i < THREADCOUNT; i  ) {
        thread_array[i].shared_ptr = shared;
        thread_array[i].threadNum = i;
    }

    /* Start threads */
    pthread_t threads[THREADCOUNT];
    for (int i = 0; i < THREADCOUNT; i  ) {
        pthread_create(&threads[i], NULL, test_process, thread_array   i);
    }

    /* Join threads */
    for (int i = 0; i < THREADCOUNT; i  ) {
        pthread_join(threads[i], NULL);
    }

    /* Test */
    for (int i = 0; i < THREADCOUNT; i  ) {
        printf("Test: %s\n", shared->test_array[i].testc);
    }

    return 0;
}