Why iam getting memory segment fault when i'm runing this C program-CodePudding

I have 2 threads and they should use the same memory. Main method should start both threads. Trå A must read the contents of a file and share it with Trå B. Trå B must also receive the data that Trå A has shared and loop through and count the number of bytes in the file. Both Threads run but on the last step before the program terminates before I memory segment fault. I use Semaphore to communicate between the Threads. here i my code:

#include <stdio.h>
#include <pthread.h>
#include <stdlib.h>
#include <unistd.h>
#include <semaphore.h>
#define BUFFER_SIZE 4096
typedef struct _Buffer
{
    int size;
    char data[BUFFER_SIZE];
} Buffer;

sem_t task1, task2;

void *thread_A(void *arg);
void *thread_B(void *arg);
int main(int argc, char *argv[])
{
    
    Buffer *memory = malloc(sizeof(Buffer));

    sem_init(&task1, 0, 0);
    sem_init(&task2, 0, 0);

    pthread_t thread_A_id;
    pthread_t thread_B_id;

    pthread_create(&thread_A_id, NULL, &thread_A,  &memory);

    pthread_create(&thread_B_id, NULL, &thread_B,  &memory);
    
    if (pthread_join(thread_A_id, NULL) != 0)
    {
        perror("Error joining thread A");
        exit(1);
    }

    if (pthread_join(thread_B_id, NULL) != 0)
    {
        perror("Error joining thread B");
        exit(1);
    }

    free(memory);
    return 0;
}

void *thread_A(void *arg)
{
    Buffer *buffer = (Buffer*) arg;

    FILE *pdf_file = fopen("file.pdf", "rb");
    if (pdf_file == NULL)
    {
        perror("Can not open the file");
    }

    printf("size of struct %ld\n", sizeof(Buffer));
    buffer->size = fread(&buffer->data, sizeof(char), BUFFER_SIZE, pdf_file);

    fclose(pdf_file);

    sem_post(&task1); 
   
    sem_wait(&task2);
    printf("A is out\n");
    return NULL;
}

void *thread_B(void *arg)
{
    printf("IAM IN TREAD B");
    Buffer *buffer = (Buffer*) arg;
    
    sem_wait(&task1);
    int i=0;;

    int byte_counts[256] = {0};
    while (buffer->size != i) {
                    
        unsigned char byte = buffer->data[i];
        byte_counts[byte]  ;

        i  ;
    }

    for (int i = 0; i < 256; i  )
    {
        printf("Byte-value X: %d\n", i, byte_counts[i]);
    }

    sem_post(&task2);

    printf("threadB is done 2\n");
    return NULL;
}

CodePudding user response：

memory is a pointer to a Buffer (Buffer *), and by taking its address, you get a pointer to a pointer to a buffer (Buffer **):

    Buffer *memory = malloc(sizeof(Buffer));
    ...
    pthread_create(&thread_A_id, NULL, &thread_A,  &memory);
    pthread_create(&thread_B_id, NULL, &thread_B,  &memory);

But in the thread functions, you're assuming that arg is a Buffer *:

    Buffer *buffer = (Buffer*) arg;

This causes undefined behaviour.

Clearly there's one indirection too many; memory is already a pointer so we don't need to take its address:

    pthread_create(&thread_A_id, NULL, &thread_A,  memory);
    pthread_create(&thread_B_id, NULL, &thread_B,  memory);

CodePudding user response：

If file fails to open, fread will return -1 and it's not checked. So the loop in thread_B will read first garbage from buffer->data and then will continue out of limit (because of comparison with -1).

So, at first, there is missing handling of error from fopen() - thread_a continues after perror, second - missing error check after fread().

By the way, the check for if (buffer->size == i) after while (buffer->size != i) is superfluous :)