Memory leak when creating & destroying threads

Question

I'm writing a program that performs some processing on each packet received. I've taken some of the multithreading code and put it into its own program:

#include <stdio.h>
#include <math.h>
#include <stdlib.h>
#include <string.h>
#include <pthread.h>
#include <signal.h>
#include <unistd.h>

struct pktstruct {
    //u_char *args; - Not used
    const struct pcap_pkthdr *head;
    const u_char *packt;
    int thread_no;
};
void* packet_handler(void *arguments);
void handler_dispatcher(u_char *args, const struct pcap_pkthdr *header, const u_char *packet);
pthread_t threads[20];
volatile sig_atomic_t thread_done[20];

int main(int argc, char *argv[])
{
    for (int i = 0; i < 20; i ++){
        thread_done[i] = 0;
    }

    while (1) {
        handler_dispatcher(NULL,NULL,NULL);
    }
}

void* packet_handler(void *arguments)
{
    struct pktstruct *args = arguments;
    printf("Hello %d\n", args->thread_no);
    thread_done[args->thread_no] = 1;
    return NULL;
}

void handler_dispatcher(u_char *args, const struct pcap_pkthdr *header, const u_char *packet){
    struct pktstruct pkt;
    pkt.head = header;
    pkt.packt = packet;
    for (int t = 0; t < 20; t++) {
        if ( thread_done[t] ) {
            //printf("Thread %d is done! Joining...\n", t);
            pthread_join(threads[t], NULL);
            thread_done[t] = 0;
        }
    }
    for (int q = 0; q < 20; q++) {
        if ( !thread_done[q] ) {
            pkt.thread_no = q;

            pthread_create(&threads[q], NULL, &packet_handler, &pkt);
            break;
        }
    }
}

Originally I was just spawning a thread for each packet. I'm not very familiar with C so it took me a while to realize stuff like threads don't clean up, return NULL is better than pthread_exit(NULL) etc.

The program prints a bunch of "Hello"s and then stops functioning properly. HTOP shows 256G for VIRT and 266M for RES, but the only reason they stop growing is because the program stops working.

I understand that it would be 'good design' to use mutexes, however I thought that since I'm checking for free slots in each iteration, surely even if I miss a thread just becoming available, next iteration I will know about it.

To be fully honest, I tried using mutexes too -locking and unlocking before and after the 'for' loops in handler_dispatcher, as well as before and after the assignment in packet_handler- but the program again stops working after a brief moment with 256G VIRT and 266M RES.

What is it that I'm doing wrong?

Edit: Added headers. Compile with gcc test.c -o test -Wall -lpthread

Answer 1

You never ever use volatile variables to communicate between threads. volatile does not imply atomic. Accessing a non-atomic variable from more than one thread is undefined. But let's pretend for the sake of learning that this is not an issue, and look at your two loops.

First loop:

 for (int t = 0; t < 20; t++) {
       if ( thread_done[t] ) {
           //printf("Thread %d is done! Joining...\n", t);
           pthread_join(threads[t], NULL);
           thread_done[t] = 0;
       }
  }

At first call of handler_dispatcher, there are no threads yet, and all thread_done members are 1. You are calling pthread_join 20 times on something that are not valid threads. This is undefined, but suppose you have survived this somehow. You set all thread_done to 0.

Second loop:

for (int q = 0; q < 20; q++) {
    if ( !thread_done[q] ) {
        pkt.thread_no = q;

        pthread_create(&threads[q], NULL, &packet_handler, &pkt);
        break;
    }
}

When the function is called for the first time, this loop creates a single thread.

When the function is called for the second time, that thread is probably still running, so the first loop does nothing. (printf is a slow function that does IO, so it is safe to assume it takes 1000s times more time than a single iteration of your while(1) loop in main).thread_doneis zero, so you create a new thread. The new thread ID overwrites the originalpthread_t` object, which is lost forever, so the original thread is impossible to join.

When the function is called for the third time, the second thread you have created is probably still running. So you create a new thread. The new thread ID overwrites the original pthread_t object, which is lost forever, so the original thread is impossible to join.

When the function is called for the fourth time, the third thread you have created is probably still running. So you create a new thread. The new thread ID overwrites the original pthread_t object, which is lost forever, so the original thread is impossible to join.

---

When the function is called for the 5380th time, the very first thread you have created has finally terminated. It sets its thread_done flag, and your first loop does a join. Only it joins not the first thread, but 5379th. This call blocks until that thread is done too. Then the sequence restarts.

All in all, I think this sequence of events is probably not quite what you want. Passing pointers to local variables around is but a cherry on top of this pie. I suggest getting a proper cookbook.

---

This was written when thread_done[i] = 0; was thread_done[i] = 1; but this doesn't change the outcome too much.