Repository: godot
Issue: `SafeBinaryMutex::try_lock()` lets multiple threads in
Labels: bug,topic:core,needs testing

### Godot version

4.2

### System information

Windows 11

### Issue description

I was examining potential avenues for a fix for https://github.com/godotengine/godot/issues/84137#issuecomment-1793723728

and noticed this code in SafeBinaryMutex.

```cpp
_ALWAYS_INLINE_ bool try_lock() const {
		if (count) {
			count++;
			return true;
		} else {
			if (mutex.try_lock()) {
				count++;
				return true;
			} else {
				return false;
			}
		}
	}
```
as well as this:
```cpp
	~SafeBinaryMutex() {
		DEV_ASSERT(!count);
	}
```
versus this destructor in `MutexLock`:
```cpp
	_ALWAYS_INLINE_ ~MutexLock() {
		SafeBinaryMutex<Tag>::count--;
	};
```
(there's a `std::unique_lock` involved which will be released when `~MutexLock()` runs, but this destructor will still silently succeed if `count!=0` as if everything was a-ok)

### Nature of Race Condition
This constellation means that if any other thread gets a hold of this `SafeBinaryMutex` in a locked state (`count>0`), and calls `try_lock`, it will return `true` (should return `false`). 

This bug also circuvents the `std::unique_lock<std::mutex>>` in the widely used `MutexLock` semantic (if that uses a `SafeBinaryMutex`, but it appears that the majority of these do).

### Steps to reproduce

```cpp
SafeBinaryMutex<MY_TAG> our_mutex;

//---------------------------------------
//Thread 1: uses MutexLock semantic
//New scope
{
    MutexLock foo(our_mutex);
    // Thread 1 does thread sensitive stuff
    //...
    // Thread 2 becomes active
    //...
    // Thread 1 finishes thread sensitive work
    //...
} // BUG HAPPENS HERE!
// MutexLock went out of scope, unlocking the std::unique_lock in it, but count is 1 (Thread 2 has not unlocked it yet)!

//... Thread 3 becomes active
//New Scope
{
    MutexLock bar(our_mutex); //succeeds silently, because a new std::unique_lock is used
    // Thread 1 does thread sensitive stuff DESPITE Thread 3 assuming it has the mutex lock!
    //... SafeBinaryMutex provides no safety guarantee at all in this state


//---------------------------------------
//Thread 2: uses SafeBinaryMutex semantic
if (our_mutex.try_lock()) // BUG HAPPENS HERE!
{
    //... execution enters freely because count > 0
    // Thread 2 performs thread sensitive work
    //...
    // Thread 1 becomes active.
    //...
    // Thread 2 finishes up thread seensitive work
    //...
    our_mutex.unlock();
}

//---------------------------------------
//Thread 3: uses SafeBinaryMutex semantic
our_mutex.lock(); //Silently succeeds: ++count is 2, so now the associated std::mutex isn't even locked!
// Thread 3 does thread sensitive stuff
// ...
// Thread 1 becomes active.
// ...
// Thread 3 finishes work
// ...
our_mutex.unlock();
```

## Suggested patch:
(for discussion, I may totally overlook something here, this stuff is hard and used all over the engine)
```patch
diff --git a/core/os/mutex.h b/core/os/mutex.h
index cee0f8af74..4f0c0e6730 100644
--- a/core/os/mutex.h
+++ b/core/os/mutex.h
@@ -93,16 +93,11 @@ public:
        }

        _ALWAYS_INLINE_ bool try_lock() const {
-               if (count) {
+               if (mutex.try_lock()) {
                        count++;
                        return true;
                } else {
-                       if (mutex.try_lock()) {
-                               count++;
-                               return true;
-                       } else {
-                               return false;
-                       }
+                       return false;
                }
        }

@@ -137,6 +132,7 @@ public:
        };
        _ALWAYS_INLINE_ ~MutexLock() {
                SafeBinaryMutex<Tag>::count--;
+               DEV_ASSERT(!SafeBinaryMutex<Tag>::count);
        };
 };
```

However, I'd rather recommend using `std::unique_lock` here, too, because it is safer for recursive locks and a thread (especially User code in GDScript) may want to `try_lock()` repeatedly. This is a risk because `std::mutex::try_lock` has undefined behaviour if it is called by the same thread twice. This would probably imply changing `SafeBinaryMutex` into something that inherits from / has a `std::unique_lock` instead of letting other classes rawdog its own internal mutex and count variables.


### Minimal reproduction project

don't have one right now, but see https://github.com/godotengine/godot/issues/84137#issuecomment-1793723728 for one possible candidate that runs into a race condition that may or may not be associated by `SafeBinaryMuted::try_lock()`.

https://github.com/godotengine/godot/files/13259418/jittery-vsync-bug-repro.zip