Is the following C function thread-safe?

Question

I saw a blog stating the below code is thread safe , but the condition count not being inside the mutex would cause a data corruption; in case two threads check the count at the same time but before either acquires a mutex and one acquire the mutex after contending. When one thread completes , the other would very blindly add one more value to the array.

char arr[10];
int count=0;

int func(char ctr)
{
    int i=0;
    if(count >= sizeof(arr))
    {
        printf("\n No storage\n");
        return -1;
    }

     /* lock the mutex here*/

    arr[count] = ctr;
    count++;

    /* unlock the mutex here*/

    return count;
}

---

Would I be right if I made the following changes? Or is there a better/efficient way to do it

   int func(char ctr)
    {
    int i=0;

    /* lock the mutex here*/

    if(count >= sizeof(arr))
    {
        printf("\n No storage\n");

        /* unlock the mutex here*/

        return -1;
    }


    arr[count] = ctr;
    count++;

    /* unlock the mutex here*/

    return count;
}`

Answer 1

You are correct. By doing the check outside of the critical section you are opening the doors for a possible buffer overrun. However, note that the returned count may not be the same index used to store ctr. That's an issue even in the corrected code.

In order to remedy that you could rewrite it like this:

int func(char ctr)
{
    /* lock the mutex here*/

    if(count >= sizeof(arr)) {
        printf("\n No storage\n");

        /* unlock the mutex here*/

        return -1;
    }


    arr[count] = ctr;
    int c = count++;

    /* unlock the mutex here*/

    return c;
}

It's worth noting that, if that's the only function changing "count", then no two threads would be able to change the same memory position in arr and this would actually be safe:

int func(char ctr)
{
    /* lock the mutex here*/

    if(count >= sizeof(arr)) {
        printf("\n No storage\n");

        /* unlock the mutex here*/

        return -1;
    }

    int c = count++;

    /* unlock the mutex here*/

    arr[c] = ctr;

    return c;
}

If that's the pattern, maybe you can refactor that code into two functions, like so:

int get_sequence(void)
{
    /* lock the mutex here*/

    int c = count++;

    /* unlock the mutex here*/

    return c;
}

int func(char ctr)
{
    int c = get_sequence();
    if(c >= sizeof(arr)) {
        printf("\n No storage\n");
        return -1;
    }

    arr[c] = ctr;

    return c;
}

Note that will only work as long as get_sequence is really the only function changing count variable.