Segmentation Fault - Producer Consumer

Question

This is my basic code , now the problem is that it runs for couple of loops and then gives segmentation fault. Now I know that segmentation error is due to illegal read/write at a memory location, but I haven't used any pointers on that note.

#include<stdio.h>
#include<math.h>
#include<stdbool.h>
#include<pthread.h>
#include<stdlib.h>

int counter = 0;
int BUFFER_SIZE = 5;
int buffer[] = {0};
int in = 0;
int out = 0;
void *prod(char);
void *cons(void);
bool flag = true;

void main()
{   int i, j;

    pthread_t thread1, thread2; 
    do{

            flag = true;
            i = pthread_create(&thread1, NULL, prod('i'), NULL);
            flag = true;
            j = pthread_create(&thread2, NULL, cons(), NULL);

    }while(1);
}

void* prod(char a)
{
    while (flag) {
      printf("\nCounter  = %d", counter);

while (counter == BUFFER_SIZE) {
      printf("\nBusy Waiting!!!");
} 

buffer[in] = a;
in = (in + 1) % BUFFER_SIZE;
printf("\nProducer produced an item %c!",a);
counter++;
printf("\nTotal items produced = %d",counter);

flag = false;

}
}

void* cons()
{
  char a;
  while (flag) {
    printf("\nCounter  = %d",counter);
    while (counter == 0){printf("\nBusy Waiting!!!");
  } 

  a = buffer[out];
  out = (out + 1) % BUFFER_SIZE;

  counter--;

  printf("\nTotal items remaining = %d",counter);
  flag = false;
}
}

OUPUT

You are creating infinitely many threads. Two every iteration to be exact. — StoryTeller - Unslander Monica, Mar 14 '17 at 14:07
But the error I am getting is after 1 or 2 times the function is called @StoryTeller — Gunjan Raval, Mar 14 '17 at 14:09
No, you get it after you saw input about two times. Thread creation can happens much faster than that. That's a big mistake with your program. — StoryTeller - Unslander Monica, Mar 14 '17 at 14:10
And you need to provide a function pointer to a thread procedure, while your code calls these functions (`cons()`) and uses their results as pointers to thread functions. Neither cons nor prod return anything which makes a return value undefined. When pthread_create starts a thread with such pointer to function, it simply crashes. — Maksim Skurydzin, Mar 14 '17 at 14:11
Ok so will I have to create the thread only once and then run it ? @StoryTeller — Gunjan Raval, Mar 14 '17 at 14:12
A producer consumer model involves a finite number of threads, that use **proper synchronization** to access an abstract queue in a loop. The in-a-loop part means the threads execute a loop, not that they are created in a loop without stop. — StoryTeller - Unslander Monica, Mar 14 '17 at 14:15
@MaksimSkurydzin Then how are they running once or twice and then giving error ? Shouldn't they give error then and there ? — Gunjan Raval, Mar 14 '17 at 14:27
@MaksimSkurydzin pthread accepts functions with return type (void*) — Gunjan Raval, Mar 14 '17 at 14:42
Overall, you need to actually study how threads work before using them... This program has so many problems. — Lundin, Mar 14 '17 at 14:46
@GunjanRaval `pthread_create(&thread1, NULL, prod, NULL);` After the threads are created, you can wait on their completion using `pthread_join` on each of them — Maksim Skurydzin, Mar 14 '17 at 14:46

score 1 · Accepted Answer · edited May 23 '17 at 12:09

You have multiple severe bugs:

You are creating threads in an eternal loop, until the program runs out of memory. What you want to do is to just create n threads once, then let the main program loop (forever?) after that.
pthread_create(&thread1, NULL, prod('i'), NULL) is incorrect, you are calling the callback function here instead of providing a function pointer to it. Arguments to the callback need to be passed separately. Read the manual about pthread_create.
pthreads expect a function format of the type void* func (void*). You are not allowed to use any other function format. So both of your callback functions have the wrong format.
You aren't using any form of protection mechanism for variables shared between multiple threads. You need to use a mutex or similar.
stdio.h is not necessarily thread-safe, depending on which system and C standard version you are using. See stdout thread-safe in C.

Thanks Man ! Got it I wasn't using the function format correctly !. — Gunjan Raval, Mar 14 '17 at 14:53
@GunjanRaval Please note that you need to fix _all_ of the above concerns. These aren't some minor nit-picks but all of them are severe bugs. — Lundin, Mar 14 '17 at 14:59

score 0 · Answer 2 · answered Mar 14 '17 at 14:13

0

You should run the loop one time and use pthread_join to wait for created thread(s). The while loop creates new threads and re-write the handles (thread1 and thread2) which is most probably what is causing crash.

answered Mar 14 '17 at 14:13

sardok

1,086
1
10
19

But I want these threads to run infinitely, then what can I do ? – Gunjan Raval Mar 14 '17 at 14:15
Overwritting the `pthread_t` variables is no problem, it is only some kind of integer to identify the thread. The real problem are the function calls in `pthread_create(..)`. – mch Mar 14 '17 at 14:21

L. Alberto Giménez · Answer 3 · 2017-03-15T14:10:04.563

Your segfault is mostly sure due to the buffer array. You are defining an array of size 1 and later in the code you are using up to 5 positions.

You are defining it as:

int BUFFER_SIZE = 5;
int buffer[] = {0};

That actually creates a buffer that can only hold 1 int (because you are using an initializer with just one value).

Then you are indexing it modulo BUFFER_SIZE:

buffer[in] = a;
in = (in + 1) % BUFFER_SIZE;

And that will overflow buffer after the first iteration (when in is greater than 0, and you are trying to index a position that is not allocated -or, better said, part of the memory allocated for buffer).

You think that you aren't using pointers, but under the hoods, you are. In C, the array indexing buffer[in] is equivalent to *(buffer + in), so you are in fact "using pointers" in your code.

Segmentation Fault - Producer Consumer

3 Answers3