C multi-thread malloc bad access global

Question

I have 2 structs as follows:

typedef struct _product
{
    unsigned int     product_id;
    time_t           my_time_stamp;
    unsigned int     lifespan;
} product;

typedef struct _queue
{
    product *   elem;
    struct _queue * next;
} queue;

I have a global variable head.

queue *head; 

In main I malloc the head of the queue.

head = (queue *)malloc(sizeof(queue));

And Call it in another function

    if(head->elem == NULL) { head = newPointer; }

When I malloc the head everything is fine. Then when it jumps to the function it resets to 0x0.

Is this the wrong way to do this? If so how should it be done?

void producer_func(void *tid)
{   
  while(number_of_products_created < number_of_products_max) //check for completeness of task
  {
    my_str("in producer with id ");
    my_int((long)tid);
    br();
    pthread_mutex_lock(&the_mutex);
    my_str("locked by ");
    my_int((long)tid);
    br();
    while(space_in_queue >= size_of_queue)
      pthread_cond_wait(&notFull, &the_mutex); //check to see if there is room in the queue

    product *tempProduct = malloc(sizeof(product));

    //////////////enter critical region/////////////
    tempProduct->product_id = product_id_count++;
    //////////////exit critical region/////////////

    //time
    struct timeval tim;
    gettimeofday(&tim, NULL);
    tempProduct->my_time_stamp = tim.tv_sec; 
    //endtime
    tempProduct->lifespan = rand();
    //new item for queue
    queue *newPointer = malloc(sizeof(queue));
    newPointer->elem = tempProduct;
    newPointer->next = NULL;                

    //critical region//
    if(head == NULL)
    {
      head = newPointer;
    }
    else
    {
      //traverse list
      queue *tempPointer;
      tempPointer = head;
      while(tempPointer->next != NULL)
      {
        tempPointer = tempPointer->next;
      }
      tempPointer->next = newPointer;
    }
    space_in_queue++;
    number_of_products_created++;
    //end critical region//

    my_str("unlocked by ");
    my_int((long)tid);
    br();
    usleep(10000);
    my_str("num products created is ");
    my_int(number_of_products_created);
    br();
    usleep(10000);
    pthread_cond_broadcast(&notEmpty);
    pthread_mutex_unlock(&the_mutex);
    usleep(100000); //let others have a chance
  }
}

Answer 1

I suppose that head is a global variable. No idea why your function doesn't find a good value in there, but you could avoid all your problems by just not having it a pointer variable:

queue head = {0};

Then your list would always be well defined and you'd never even have to check it. Your semantics for your list being empty or not would change though.

And Jonathan is right that you have an awful lot of code in your critical section. In particular, Sleeping inside such a section is really a bad idea.

Answer 2

That's an awful lot of code between locking the mutex and releasing it. You should aim to minimize the amount of work done with the lock held. You should do all the work except add the item to the list without the lock. Only when everything is ready do you grab the mutex and go ahead with adding the item and release the mutex. It might be a good idea to make adding the item at the end of the list an O(1) rather than O(N) operation too - keep a pointer to the tail of the structure. You really won't get good performance (concurrency) out of the system if you sleep for 10 milliseconds at a time with the mutex held locked.

Another critique is that your product structure will occupy 24 bytes on many 64-bit platforms, rather than just 16. If time_t is an 8-byte quantity and 8-byte quantities are aligned on 8-byte boundaries, you waste 4 bytes after each of the unsigned int values.

However, that's just general critique - not directly a source of your problem.

---

Substantive

You do not seem to be initializing the queue which you allocate with malloc(). Since malloc() returns uninitialized data, it can contain anything. Before you do anything else, you must turn it into a properly formed queue item - with initialized pointers etc.