Thread-safe Shared Pointer implementation in C

Question

I've been working on a C project where I needed a small, thread-safe shared pointer, and so I wrote this implementation.

It's primary use in the project is as a way of allowing multiple threads to have ownership of a resource at once. This means it's very important that it is thread safe (and also doesn't leak, obviously)

SharedPtr.h

#ifndef SHARED_PTR_H
#define SHARED_PTR_H

typedef struct shared_ptr* SharedPtr;

typedef void(*RawPtrDestructor_t)(void*);

SharedPtr SharedPtr_create(void* rawPtr, RawPtrDestructor_t destructorFunc);

SharedPtr SharedPtr_copy(restrict SharedPtr rhs);

void* SharedPtr_get(restrict SharedPtr sharedPtr);

#ifndef SHARED_PTR_NO_THREAD_SAFETY
void SharedPtr_lockMutex(restrict SharedPtr sharedPtr);

void SharedPtr_unlockMutex(restrict SharedPtr sharedPtr);
#endif //SHARED_PTR_NO_THREAD_SAFETY

void SharedPtr_free(SharedPtr sharedPtr);

#endif //SHARED_PTR_H

SharedPtr.c

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#ifndef SHARED_PTR_NO_THREAD_SAFETY
#include <pthread.h>
#endif //SHARED_PTR_NO_THREAD_SAFETY
#include "SharedPtr.h"

struct shared_ptr
{
    struct shared_ptr* previous;

    struct shared_ptr* next;

    #ifndef SHARED_PTR_NO_THREAD_SAFETY
    pthread_mutex_t* sharedMutex;
    #endif //SHARED_PTR_NO_THREAD_SAFETY

    void* rawPtr;

    RawPtrDestructor_t destructor;
};

static void genericDestructor(void* ptr)
{
    free(ptr);
    ptr = NULL;
}

static SharedPtr mallocSharedPtr(void)
{
    //Create the shared pointer
    SharedPtr sharedPtr = malloc(sizeof(struct shared_ptr));
    //Check to make sure the malloc didn't fail
    if (!sharedPtr)
    {
        fprintf(stderr, "Alloc error at %s:%i. Aborting...\n", __FILE__, __LINE__);
        abort();
    }
    return sharedPtr;
}

SharedPtr SharedPtr_create(void* rawPtr, RawPtrDestructor_t destructorFunc)
{
    //Create the shared pointer
    SharedPtr sharedPtr = mallocSharedPtr();

    #ifndef SHARED_PTR_NO_THREAD_SAFETY
    //Create the mutex
    int result = pthread_mutex_init(sharedPtr->sharedMutex, NULL);
    //Check that it didn't fail
    if (result != 0)
    {
        fprintf(stderr, "Pthread mutex init error '%s' at %s:%i. Aborting...\n", strerror(result), __FILE__, __LINE__);
        abort();
    }
    #endif //SHARED_PTR_NO_THREAD_SAFETY

    //Set the raw pointer
    sharedPtr->rawPtr = rawPtr;
    //If the destructor isn't specified, use the generic destructor
    if (!destructorFunc) {sharedPtr->destructor = &genericDestructor;}
    //Otherwise just use the one that's given
    else {sharedPtr->destructor = destructorFunc;}

    //Make sure previous and next are NULL
    sharedPtr->previous = NULL;
    sharedPtr->next = NULL;

    //Return the shared pointer
    return sharedPtr;
}

SharedPtr SharedPtr_copy(restrict SharedPtr rhs)
{
    //Create the shared pointer
    SharedPtr newSharedPtr = mallocSharedPtr();

    #ifndef SHARED_PTR_NO_THREAD_SAFETY
    //Set the mutex
    newSharedPtr->sharedMutex = rhs->sharedMutex;

    //Lock the mutex
    pthread_mutex_lock(rhs->sharedMutex);
    #endif //SHARED_PTR_NO_THREAD_SAFETY

    //Set the raw pointer and destructor
    newSharedPtr->rawPtr = rhs->rawPtr;
    newSharedPtr->destructor = rhs->destructor;

    //Set the 'previous' pointer to this shared pointer
    newSharedPtr->previous = rhs;

    //Set the 'next' pointer to NULL
    newSharedPtr->next = NULL;

    //Set the 'next' pointer of this shared pointer to the new shared pointer
    rhs->next = newSharedPtr;

    #ifndef SHARED_PTR_NO_THREAD_SAFETY
    //Unlock the mutex
    pthread_mutex_unlock(rhs->sharedMutex);
    #endif //SHARED_PTR_NO_THREAD_SAFETY

    //Return the new shared pointer
    return newSharedPtr;
}

void* SharedPtr_get(restrict SharedPtr sharedPtr)
{
    #ifndef SHARED_PTR_NO_THREAD_SAFETY
    //Create a variable to store the pointer
    void* ret = NULL;
    //Lock the mutex
    pthread_mutex_lock(sharedPtr->sharedMutex);  //Is the mutex locking/unlocking here even necessary?
    //Set the pointer
    ret = sharedPtr->rawPtr;
    //Unlock the mutex
    pthread_mutex_unlock(sharedPtr->sharedMutex);
    //Return the pointer
    return ret;
    #else
    //Return the raw pointer
    return sharedPtr->rawPtr;
    #endif //SHARED_PTR_NO_THREAD_SAFETY
}

#ifndef SHARED_PTR_NO_THREAD_SAFETY
void SharedPtr_lockMutex(restrict SharedPtr sharedPtr)
{
    //Lock the shared mutex
    pthread_mutex_lock(sharedPtr->sharedMutex);
}

void SharedPtr_unlockMutex(restrict SharedPtr sharedPtr)
{
    //Unlock the shared mutex
    pthread_mutex_unlock(sharedPtr->sharedMutex);
}
#endif //SHARED_PTR_NO_THREAD_SAFETY

void SharedPtr_free(restrict SharedPtr sharedPtr)
{
    #ifndef SHARED_PTR_NO_THREAD_SAFETY
    //Lock the mutex
    pthread_mutex_lock(sharedPtr->sharedMutex);
    #endif //SHARED_PTR_NO_THREAD_SAFETY

    //If this is the last shared pointer
    if (!sharedPtr->previous && !sharedPtr->next)
    {
        //Call the destructor on the raw pointer
        sharedPtr->destructor(sharedPtr->rawPtr);

        #ifndef SHARED_PTR_NO_THREAD_SAFETY
        //Unlock the mutex
        pthread_mutex_unlock(sharedPtr->sharedMutex);

        //Destroy the mutex
        int result = pthread_mutex_destroy(sharedPtr->sharedMutex);
        if (result != 0)
        {
            fprintf(stderr, "Pthread mutex destroy error '%s' at %s:%i. Aborting...\n", strerror(result), __FILE__, __LINE__);
            abort();
        }
        //Free the mutex
        free(sharedPtr->sharedMutex);
        #endif //SHARED_PTR_NO_THREAD_SAFETY
    }
    //If there are more shared pointers
    else
    {
        //Remove the pointer from the linked list
        if (sharedPtr->next) {sharedPtr->next->previous = sharedPtr->previous;}
        if (sharedPtr->previous) {sharedPtr->previous->next = sharedPtr->next;}

        #ifndef SHARED_PTR_NO_THREAD_SAFETY
        //Unlock the mutex
        pthread_mutex_unlock(sharedPtr->sharedMutex);
        #endif //SHARED_PTR_NO_THREAD_SAFETY
    }

    //Free the shared pointer's memory
    free(sharedPtr);
    sharedPtr = NULL;
}

I decided to use void pointers rather than a macro system (where calling a macro defines a version of the shared pointer with a specific pointer type) as I feel it's simpler and easier to write.

And a little example to showcase what it does:

#include "SharedPtr.h"
#include <stdio.h>
#include <stdlib.h>

void destructor(void* ptr)
{
    (void)ptr;
    puts("Destructor called!");
}

int main(void)
{
    void* myRawPtr = NULL;

    SharedPtr ptr1 = SharedPtr_create(myRawPtr, &destructor);
    SharedPtr ptr2 = SharedPtr_copy(ptr1);

    SharedPtr_free(ptr1);
    SharedPtr_free(ptr2);
}

As expected, the program only outputs "Destructor called!" once.

I've written code in C++ for a few years but I've only just started writing in C, so any input would be helpful!

Answer 1

This is an interesting idea! I wouldn't have thought to do this in straight C.

Don't Repeat Yourself

I found this a little confusing. You're keeping a linked list of struct shared_ptrs. Every time you copy a shared pointer, you add another node in the list. In doing so, you copy the pointer to the mutex, the raw pointer, and the destructor pointer into every new node in the list. It might be better to make the list be a separate struct from the nodes so you don't have to carry around as much stuff in each node. For example, it could look more like this:

typedef struct shared_ptr_list {
    SharedPtr    head;
    SharedPtr    tail;
    pthread_mutex_t    sharedMutex;
    RawPtrDestructor_t    destructor;
    void* rawPtr;
} shared_ptr_list;

and then nodes in the list would look like this:

typedef struct shared_ptr {
    SharedPtr    next;
    SharedPtr    prev;
    shared_ptr_list* list;
} shared_ptr;

Then in the various functions, you'll need to dereference the shared_ptr->list member to get at the raw pointer, the mutex, or the destructor. But there will only be 1 copy of each. This would be opaque to a user of the shared pointers - they'd never see the list structure. When you destroy the last shared pointer, you also destroy the list.

Bugs

Also, you have a bug in your list creation. In SharedPtr_copy(), you always set the previous member to rhs and next to NULL, but what happens if a caller does this:

SharedPtr ptr1;
...
ptr1 = <set to something>
...
SharedPtr ptr2 = SharedPtr_copy(ptr1);
...

At this point, things are fine, as ptr1->next is ptr2, and ptr2->prev is `ptr1'. But if we then do this:

SharedPtr ptr3 = SharedPtr_copy(ptr1);

we have a problem. ptr3->previous is ptr1. ptr2->previous is also ptr1. ptr1->next is ptr3 and there's no way to reach ptr2 by traversing the list.

This is why I recommended having a head and tail in my implementation. You can simply always add to the list by doing:

tail->next = newNode;
newNode->previous = tail;
newNode->next = NULL;
tail = newNode;

Handling Failure

I notice in several places if something fails you simply abort(). That seems like a really bad idea. You should definitely think about ways that you can handle errors like running out of memory without simply killing the process. It's not clear to me what could go wrong in destroying a mutex, but it reminds me of when closing a read-only file fails. There's not a lot you can do about it, but it doesn't warrant exiting the application!

Is This Mutex Lock/Unlock Necessary?

You have a comment in SharedPtr_get() asking whether you need to lock when getting the raw pointer. The answer is yes, you do. Think of this case: You have 1 copy of a shared pointer left and there are 2 threads operating on the same copy. (Maybe this violates your design, but it's not enforced in the code.) It is the last copy left in the linked list. You call SharedPtr_free() on thread 1, and at the exact same time call SharedPtr_get() on thread 2. If you don't lock on thread 2, you may access the raw pointer just as it's being freed on the other thread.

Obviously, you shouldn't be using the same copy of a shared pointer on 2 different threads, but I don't know of a way (off the top of my head) to enforce that. You can easily allocate one on one thread and then pass it over to another thread.