pw_{rpc, sync_stl}: Assert if STL mutex is misused
- Update the pw_sync_stl mutex to track whether it is locked and assert
if it's used improperly. The C++ standard states that unlocking an
unlocked mutex is undefined behavior. Neither libc++ nor libstdc++
crash when a mutex is unlocked multiple times (at least on Linux).
- Annotate the stub pw::rpc::internal::LockGuard class with thread
safety annotations so that thread safety analysis applies when
compiling without RPC locking.
Change-Id: Ibcd77e90aedafaa5aa05016e9657f2b6983104a2
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/77504
Pigweed-Auto-Submit: Wyatt Hepler <hepler@google.com>
Reviewed-by: Keir Mierle <keir@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Commit-Queue: Wyatt Hepler <hepler@google.com>
diff --git a/pw_rpc/public/pw_rpc/internal/lock.h b/pw_rpc/public/pw_rpc/internal/lock.h
index 617eeae..efb0443 100644
--- a/pw_rpc/public/pw_rpc/internal/lock.h
+++ b/pw_rpc/public/pw_rpc/internal/lock.h
@@ -11,7 +11,6 @@
// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
// License for the specific language governing permissions and limitations under
// the License.
-
#pragma once
#include "pw_rpc/internal/config.h"
@@ -40,9 +39,12 @@
constexpr void unlock() PW_UNLOCK_FUNCTION() {}
};
-class LockGuard {
+class PW_SCOPED_LOCKABLE LockGuard {
public:
- constexpr LockGuard(RpcLock&) {}
+ constexpr LockGuard([[maybe_unused]] RpcLock& mutex)
+ PW_EXCLUSIVE_LOCK_FUNCTION(mutex) {}
+
+ ~LockGuard() PW_UNLOCK_FUNCTION() = default;
};
#endif // PW_RPC_USE_GLOBAL_MUTEX
diff --git a/pw_sync/public/pw_sync/mutex.h b/pw_sync/public/pw_sync/mutex.h
index 622e40f..eefbed6 100644
--- a/pw_sync/public/pw_sync/mutex.h
+++ b/pw_sync/public/pw_sync/mutex.h
@@ -69,6 +69,13 @@
native_handle_type native_handle();
+ protected:
+ // Expose the NativeMutex directly to derived classes (TimedMutex) in
+ // case implementations use different types for backend::NativeMutex and
+ // native_handle().
+ backend::NativeMutex& native_type() { return native_type_; }
+ const backend::NativeMutex& native_type() const { return native_type_; }
+
private:
// This may be a wrapper around a native type with additional members.
backend::NativeMutex native_type_;
diff --git a/pw_sync_stl/BUILD.bazel b/pw_sync_stl/BUILD.bazel
index 2d5113b..bb1a247 100644
--- a/pw_sync_stl/BUILD.bazel
+++ b/pw_sync_stl/BUILD.bazel
@@ -106,9 +106,11 @@
pw_cc_library(
name = "mutex",
+ srcs = ["mutex.cc"],
target_compatible_with = select(TARGET_COMPATIBLE_WITH_HOST_SELECT),
deps = [
":mutex_headers",
+ "//pw_assert",
"//pw_sync:mutex_facade",
],
)
diff --git a/pw_sync_stl/BUILD.gn b/pw_sync_stl/BUILD.gn
index 0b7e56b..f3f07c2 100644
--- a/pw_sync_stl/BUILD.gn
+++ b/pw_sync_stl/BUILD.gn
@@ -91,6 +91,9 @@
"public_overrides/pw_sync_backend/mutex_native.h",
]
public_deps = [ "$dir_pw_sync:mutex.facade" ]
+ deps = [ dir_pw_assert ]
+
+ sources = [ "mutex.cc" ]
}
# This target provides the backend for pw::sync::TimedMutex.
diff --git a/pw_sync_stl/CMakeLists.txt b/pw_sync_stl/CMakeLists.txt
index bf8d343..b50bb38 100644
--- a/pw_sync_stl/CMakeLists.txt
+++ b/pw_sync_stl/CMakeLists.txt
@@ -17,4 +17,8 @@
pw_add_module_library(pw_sync_stl.mutex_backend
IMPLEMENTS_FACADES
pw_sync.mutex
+ PUBLIC_DEPS
+ pw_assert
+ SOURCES
+ mutex.cc
)
diff --git a/pw_sync_stl/mutex.cc b/pw_sync_stl/mutex.cc
new file mode 100644
index 0000000..177dc06
--- /dev/null
+++ b/pw_sync_stl/mutex.cc
@@ -0,0 +1,37 @@
+// Copyright 2022 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+
+#include "pw_sync/mutex.h"
+
+#include "pw_assert/check.h"
+#include "pw_sync_stl/mutex_native.h"
+
+namespace pw::sync {
+
+Mutex::~Mutex() {
+ PW_CHECK(!native_type_.locked, "Mutex was locked when it went out of scope");
+}
+
+namespace backend {
+
+void NativeMutex::SetLockedState(bool new_state) {
+ PW_CHECK_UINT_NE(locked,
+ new_state,
+ "Called %slock(), but the mutex is already in that state",
+ new_state ? "" : "un");
+ locked = new_state;
+}
+
+} // namespace backend
+} // namespace pw::sync
diff --git a/pw_sync_stl/public/pw_sync_stl/mutex_inline.h b/pw_sync_stl/public/pw_sync_stl/mutex_inline.h
index 88548c4..005d089 100644
--- a/pw_sync_stl/public/pw_sync_stl/mutex_inline.h
+++ b/pw_sync_stl/public/pw_sync_stl/mutex_inline.h
@@ -19,14 +19,27 @@
inline Mutex::Mutex() : native_type_() {}
-inline Mutex::~Mutex() {}
+inline void Mutex::lock() {
+ native_handle().lock();
+ native_type_.SetLockedState(true);
+}
-inline void Mutex::lock() { native_type_.lock(); }
+inline bool Mutex::try_lock() {
+ if (native_handle().try_lock()) {
+ native_type_.SetLockedState(true);
+ return true;
+ }
+ return false;
+}
-inline bool Mutex::try_lock() { return native_type_.try_lock(); }
+inline void Mutex::unlock() {
+ native_type_.SetLockedState(false);
+ native_handle().unlock();
+}
-inline void Mutex::unlock() { native_type_.unlock(); }
-
-inline Mutex::native_handle_type Mutex::native_handle() { return native_type_; }
+// Return a std::timed_mutex instead of the customized NativeMutex class.
+inline Mutex::native_handle_type Mutex::native_handle() {
+ return native_type_.mutex;
+}
} // namespace pw::sync
diff --git a/pw_sync_stl/public/pw_sync_stl/mutex_native.h b/pw_sync_stl/public/pw_sync_stl/mutex_native.h
index fb04d56..c90f44e 100644
--- a/pw_sync_stl/public/pw_sync_stl/mutex_native.h
+++ b/pw_sync_stl/public/pw_sync_stl/mutex_native.h
@@ -17,7 +17,19 @@
namespace pw::sync::backend {
-using NativeMutex = std::timed_mutex;
+// The NativeMutex class adds a flag that tracks whether the std::timed_mutex is
+// locked. The C++ standard states that misusing a mutex is undefined behavior,
+// so library implementations may simply ignore misuse. This ensures misuse hits
+// a PW_CHECK.
+struct NativeMutex {
+ // The locked state is tracked in a variable. This function asserts if the
+ // state is inconsistent (e.g. unlocking an unlocked mutex).
+ void SetLockedState(bool new_state);
+
+ std::timed_mutex mutex;
+ bool locked = false;
+};
+
using NativeMutexHandle = std::timed_mutex&;
} // namespace pw::sync::backend
diff --git a/pw_sync_stl/public/pw_sync_stl/timed_mutex_inline.h b/pw_sync_stl/public/pw_sync_stl/timed_mutex_inline.h
index 2fa904a..6ea86eb 100644
--- a/pw_sync_stl/public/pw_sync_stl/timed_mutex_inline.h
+++ b/pw_sync_stl/public/pw_sync_stl/timed_mutex_inline.h
@@ -19,12 +19,20 @@
namespace pw::sync {
inline bool TimedMutex::try_lock_for(chrono::SystemClock::duration timeout) {
- return native_handle().try_lock_for(timeout);
+ if (native_handle().try_lock_for(timeout)) {
+ native_type().SetLockedState(true);
+ return true;
+ }
+ return false;
}
inline bool TimedMutex::try_lock_until(
chrono::SystemClock::time_point deadline) {
- return native_handle().try_lock_until(deadline);
+ if (native_handle().try_lock_until(deadline)) {
+ native_type().SetLockedState(true);
+ return true;
+ }
+ return false;
}
} // namespace pw::sync
