Is the following C feature thread safe?
I've seen a blog where the code below is thread safe, but a condition count
not inside mutex
will cause data corruption; in case two threads check count
at the same time, but before either receiving a mutex
, or one of them receiving mutex
after approval. When one thread finishes, the other will very blindly add another 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;
}`
source to share
First, you are correct that the code from the blog has the potential to be written outside the array. Limit checking only works if done after the mutex was acquired.
This is how I would write the function:
bool func(char ctr)
{
bool result;
/* lock the mutex here */
if (count >= sizeof(arr))
{
result = FAILURE;
}
else
{
arr[count] = ctr;
count++;
result = SUCCESS;
}
/* unlock the mutex here */
if ( result == FAILURE )
printf("\n No storage\n");
return result;
}
The features of this code are worth noting
- Locking and unlocking a mutex appears only once in a function, and there are not statements
return
in the critical section. This makes it clear that mutexes will always be unlocked. -
printf
is outside the critical section.printf
relatively slow, and any function using a mutex should keep the mutex for as little time as possible. - IMO the function should not return a score, but should return an a
bool
indicating success or failure. Any code you need to know how many entries in the array should lock the mutex and check the count directly.
source to share
You're right. By checking outside of a critical section, you open doors for possible buffer overflows. Note, however, that the counter returned may not be the same index used to store the ctr. This is a problem even in the corrected code.
To fix this, you can 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 is worth noting that if the only function that changes "count", then neither of the two threads can change the same memory position in arr, and it will be really 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 this is a pattern, perhaps you can refactor this code into two functions, for example:
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 this will only work as long as get_sequence is the only function modifying the count variable.
source to share
Nothing wrong with the previous answers, but there is a better way. No mutex needed.
int func(char ctr) {
int c = interlocked_increment(&count);
if (c >= sizeof(arr)) {
printf("\n No storage\n");
interlocked_decrement(&count);
return -1;
}
arr[c-1] = ctr;
return c;
}
It depends on the availability of the increment and decrement locking functions, which must be provided by your operating system or a third party library.
Every value c
that is within the range is guaranteed not to be seen by any other thread and is therefore a valid slot in arr
, and a thread will not skip a slot if there is one available. The order in which the value is stored is undefined, but this applies to most other solutions as well. The maximum value reached count
if many threads are competing for storage is also undefined, and if this is a problem, a different approach may be required. The behavior, if count
decreasing, is another unknown. This is tricky, and you can always add additional constraints to make it more difficult.
Just to point out that there is another possible implementation based on the Check and Set (CSET) function. It is a function that checks if some place is equal to a value, and if so, sets another value atomically, returning true if it is. This avoids some criticism aligned to the above function.
int func(char ctr) {
for (;;) {
int c = count;
if (c >= sizeof(arr)) {
printf("\n No storage\n");
return -1;
}
if (CSET(&count, c, c+1)) {
arr[c] = ctr;
return c;
}
}
}
The C ++ Standard Atomic Operating Library contains a set of atomic_compare_exchange functions that should serve the purpose if available.
source to share