Question about comment in `acl_threadsupport.c` regarding design and race conditions

[IlanTruanovsky]

This came up because I was trying to solve a Coverity issue regarding the same code.

Regarding this comment in acl_threadsupport.c, I find it hard to see how this was properly designed. The comment suggests that the thread calling the acl_reset_condvar function may need to unlock C->waiter_mutex since it could have been locked by another thread that exited before unlocking the mutex. There are two problems that I see with this. First, according to https://linux.die.net/man/3/pthread_mutex_unlock:

If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, undefined behavior results.

Isn't this design doing exactly what the manual tells us is undefined behavior?

Also, by calling pthread_mutex_trylock, we introduce a race condition where we unlock an already unlocked mutex. For example, suppose thread A is calling acl_reset_condvar and thread B is some other thread holding the mutex.
Then, pthread_mutex_trylock in thread A does not lock the mutex. Suppose that after that function call, thread B unlocks the mutex. Now the mutex is no longer held by anyone, but thread A will try to unlock it anyways in line 300 which leads to undefined behavior.

Maybe we should assume that the mutex is already unlocked when calling acl_reset_condvar? To me, if some other thread exited while holding the mutex, that seems more like a problem in the user's code.

Resolving these issues may also resolve many Coverity issues complaining about a double unlock of the mutex (although I'm not 100% sure).

[pcolberg]

Thanks @IlanTruanovsky, your analysis looks good. acl_reset_condvar() should call neither pthread_mutex_trylock() nor pthread_mutex_unlock() which leads to undefined behaviour.

The question is what led to this workaround and how the underlying issue, if any may be solved properly. The comment says that exceptions are the reason:

fpga-runtime-for-opencl/lib/acl_threadsupport/src/acl_threadsupport.c

Lines 296 to 298 in 03ebe32

	// Try to unlock then unlock. This is done just in case it is already
	// locked, because if the mutex is locked we can't destroy it. This may
	// happen in cases where an asssert causes an exit, etc.

acl_reset_condvar() is called only once in the codebase:

fpga-runtime-for-opencl/src/acl_thread.cpp

Lines 104 to 106 in 03ebe32

	__attribute__((destructor)) static void l_global_lock_uninit() {
	acl_reset_condvar(&l_acl_global_condvar);
	}

The destructor is invoked when the runtime library is unloaded. Previously, the mutex was managed entirely manually, which meant that a mutex-holding thread would not get a chance to unlock the mutex if an unhandled exception occurred, but acl_reset_condvar() would be called nevertheless in the library destructor.

In the recent commit 334bdb7, a RAII-safe wrapper around the mutex, std::scoped_lock was introduced, which unlocks the mutex when an exception is thrown. However, assert() invokes abort(), which does not call
destructors of variables with automatic, thread local and static storage durations.

On the other hand, the following does not seem to invoke the destructor either:

#include <cassert>
#include <cstdio>
__attribute__((constructor)) static void l_global_lock_init() { printf("Hello!\n"); }
__attribute__((destructor)) static void l_global_lock_uninit() { printf("Bye!\n"); }
int main() {
  assert(0);
}

In summary, I am not sure whether the comment underlying the workaround actually holds. I suggest you take a closer look at __attribute__((destructor)) and experiment with exceptional exits to see whether this is actually an issue. If the library destructor is indeed invoked, it could be replaced with a RAII wrapper in static storage that constructs and deconstructs the condition variable; which would not be called by assert() as detailed earlier.

[IlanTruanovsky]

I see. So if I'm understanding this correctly, the main problem is that the shared library destructor is not being called during an abort.

I tried experimenting with some code. The following works, although I'm not sure if it's suitable since it involves signal handling:

#include <cassert>
#include <csignal>
#include <cstdio>

__attribute__((destructor)) static void destruct(int) { printf("Bye!\n"); }
__attribute__((constructor)) static void construct() {
  printf("Hello!\n");
  std::signal(SIGABRT, destruct);
}

int main() { assert(0); }

Additionally, throwing an exception and not catching it is also capable of calling the destructor without any signal handling.

There may be other cases that are not mentioned in the code's comment (i.e. exiting not caused due to an assert) that may be problematic and are not accounted for when using just std::signal(SIGABRT, destruct).

[pcolberg]

I see. So if I'm understanding this correctly, the main problem is that the shared library destructor is not being called during an abort.

No, the opposite. The comment underlying the workaround assumes that the library destructor is called when assert() aborts. Which would be an issue since it calls acl_reset_condvar() when the mutex is potentially locked. My impression (which I have not found documentation for yet) is that the library destructor is not called when assert() aborts. Which would be good since acl_reset_condvar() is not called when the mutex is potentially locked.

[IlanTruanovsky]

Hmm I think there are actually two scenarios I'm getting confused about here:

Scenario 1 (the one I believe you are describing):
Thread A is holding the lock, thread B is not holding the lock. Thread B is currently running and then suddenly aborts.

• Assuming the destructor gets called after thread B aborts, we get undefined behavior since thread A is still holding the lock.
• Assuming the destructor doesn't get called after thread B aborts, then everything is fine.

Scenario 2 (the one I was thinking of):
Thread A is holding the lock, thread B is not holding the lock. Thread A aborts while holding the lock.

• Assuming the destructor gets called, then everything is fine as long as thread B does not end up grabbing the mutex while thread A is destroying it.
• Assuming the destructor doesn't get called, then the mutex will be permanently held by thread A which is very bad.

On second thought, should I even be considering scenario 2? If a thread aborts while having the mutex, it's probably that thread's responsibility to make sure it releases the mutex before aborting.

[pcolberg]

Apologies, I was not being clear about my assumptions: I am assuming that we are discussing the implications of the fixed code, without undefined behaviour:

--- a/lib/acl_threadsupport/src/acl_threadsupport.c
+++ b/lib/acl_threadsupport/src/acl_threadsupport.c
@@ -293,11 +293,6 @@ void acl_reset_condvar(struct acl_condvar_s *C) {
 #else
   {
     int ret = 0;
-    // Try to unlock then unlock. This is done just in case it is already
-    // locked, because if the mutex is locked we can't destroy it. This may
-    // happen in cases where an asssert causes an exit, etc.
-    pthread_mutex_trylock(&(C->waiter_mutex));
-    pthread_mutex_unlock(&(C->waiter_mutex));
     ret |= pthread_mutex_destroy(&(C->waiter_mutex));
     ret |= sem_destroy(&(C->signal_sem));
     ret |= sem_destroy(&(C->passive_sem[0]));

The main point about assert() leading to abort() is that by invoking assert() on a false expression, the decision has been made that the program is in an unrecoverable state; there is no point in trying to free resources in an orderly manner, that task has now been delegated to the kernel. So there is no point in worrying about freeing up the mutex on abort. I just want to make sure that the comment removed above is in fact wrong; that the library destructor is not invoked on abort.

[IlanTruanovsky]

OK. So to summarize, what's left to do is just to confirm that library destructor function works as we expect it to from our example code (i.e. they are not invoked on abort), and if not, implement a RAII wrapper to handle this cleanup.

[pcolberg]

OK. So to summarize, what's left to do is just to confirm that library destructor function works as we expect it to from our example code (i.e. they are not invoked on abort)

Yes; basically to confirm our understanding of __attribute__((destructor)) in theory and in practice. A second task is to run integration testing to ensure that the non-exceptional, orderly exit is not affected by the removal of those two lines.

and if not, implement a RAII wrapper to handle this cleanup.

If not, I don't know yet what the solution would be.

[pcolberg]

@IlanTruanovsky, could you submit a PR which addresses this issue?

[IlanTruanovsky]

@IlanTruanovsky, could you submit a PR which addresses this issue?

I have not had the chance to test whether the library destructor works as expected, but sure, I can submit the code change.