[RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation

[RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation

[Alviro Iskandar Setiawan]

Hi,

The current C++ mutex implementation is undefined behavior, specifically
in the cond_wait() function because std::unique_lock<std::mutex> is
constructed with std::defer_lock while the same thread has acquired the
lock. Also, right after that defer_lock, std::condition_variable calls
wait() with a unique lock, not in a locked state.

In such a situation, the correct construction is using std::adopt_lock.
However, using std::adopt_lock leads to another issue. The issue is the
lock will be released upon return in the cond_wait(). To solve the
problem, introduce a new helper function, __cond_wait() which will
release the lock with std::adopt_lock and then call it from cond_wait().
The cond_wait() then acquires the lock again before it returns. The
result is that we correctly fulfill the __must_hold() semantic while
conforming to the C++ mutex implementation.

Signed-off-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>
---
Alviro Iskandar Setiawan (3):
  MAINTAINERS: Add myself as the thread maintainer
  core/thread: Fix undefined behavior in the C++ mutex implementation
  configure: Introduce `--cpp-thread` option

 MAINTAINERS         |  7 +++++++
 configure           |  8 ++++++++
 core/thread.cc      | 17 +++++++++++++----
 include/gw/thread.h |  2 +-
 4 files changed, 29 insertions(+), 5 deletions(-)


base-commit: 2ca56f61c307813ad7069cf08c350f2ff61fc615
-- 
Alviro Iskandar Setiawan

[RFC PATCH v1 1/3] MAINTAINERS: Add myself as the thread maintainer

[Alviro Iskandar Setiawan]

Signed-off-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0603389..6d0b0ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -69,6 +69,13 @@ Maintainers List
           first. When adding to this list, please keep the entries in
           alphabetical order.
 
+THREAD
+M:	Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>
+L:	GNU/Weeb Mailing List <gwml@vger.gnuweeb.org>
+S:	Maintained
+T:	git https://github.com/alviroiskandar/GNUWeebBot2.git
+F:	core/thread.cc
+F:	include/gw/thread.h
 
 THE REST
 M:	Ammar Faizi
-- 
Alviro Iskandar Setiawan

[RFC PATCH v1 2/3] core/thread: Fix undefined behavior in the C++ mutex implementation

[Alviro Iskandar Setiawan]

The current C++ mutex implementation is undefined behavior, specifically
in the cond_wait() function because std::unique_lock<std::mutex> is
constructed with std::defer_lock while the same thread has acquired the
lock. Also, right after that defer_lock, std::condition_variable calls
wait() with a unique lock, not in a locked state.

In such a situation, the correct construction is using std::adopt_lock.
However, using std::adopt_lock leads to another issue. The issue is the
lock will be released upon return in the cond_wait(). To solve the
problem, introduce a new helper function, __cond_wait() which will
release the lock with std::adopt_lock and then call it from cond_wait().
The cond_wait() then acquires the lock again before it returns. The
result is that we correctly fulfill the __must_hold() semantic while
conforming to the C++ mutex implementation.

Signed-off-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>
---
 core/thread.cc | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/core/thread.cc b/core/thread.cc
index 2ff81e5..54e06e7 100644
--- a/core/thread.cc
+++ b/core/thread.cc
@@ -181,13 +181,22 @@ int cond_init(struct cond_struct **c)
 	return 0;
 }
 
-int cond_wait(struct cond_struct **c, struct mutex_struct **m)
+static int __cond_wait(struct cond_struct *c, struct mutex_struct *m)
 {
-	std::unique_lock<std::mutex> lock((*m)->mutex, std::defer_lock);
-	(*c)->cond.wait(lock);
+	std::unique_lock<std::mutex> lock(m->mutex, std::adopt_lock);
+	c->cond.wait(lock);
 	return 0;
 }
 
+int cond_wait(struct cond_struct **c, struct mutex_struct **m)
+{
+	int ret;
+
+	ret = __cond_wait(*c, *m);
+	mutex_lock(m);
+	return ret;
+}
+
 int cond_signal(struct cond_struct **c)
 {
 	(*c)->cond.notify_one();
-- 
Alviro Iskandar Setiawan

[RFC PATCH v1 3/3] configure: Introduce `--cpp-thread` option

[Alviro Iskandar Setiawan]

Allow user to force use C++ thread instead of pthread on Linux. It
also allows us to debug the C++ implementation on Linux.

Signed-off-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>
---
 configure           | 8 ++++++++
 core/thread.cc      | 2 +-
 include/gw/thread.h | 2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 887b0b2..2f93056 100755
--- a/configure
+++ b/configure
@@ -40,6 +40,9 @@ for opt do
 	--sanitize)
 		use_sanitize="yes";
 	;;
+	--cpp-thread)
+		use_cpp_thread="yes";
+	;;
 	*)
 		echo "ERROR: unknown option $opt";
 		echo "Try '$0 --help' for more information";
@@ -60,6 +63,7 @@ Options: [defaults in brackets after descriptions]
   --cxx=CMD                Use CMD as the C++ compiler
   --debug                  Build with debug enabled
   --sanitize               Build with sanitize enabled
+  --cpp-thread             Force to use C++ thread implementation
 EOF
 exit 0;
 fi
@@ -308,6 +312,10 @@ if test "${use_sanitize}" = "yes"; then
 	LDFLAGS="${LDFLAGS} -fsanitize=address -fsanitize=undefined";
 fi;
 
+if test "${use_cpp_thread}" = "yes"; then
+	add_config "CONFIG_CPP_THREAD";
+fi;
+
 add_config "CONFIG_MODULE_PING";
 
 add_make_var "CC" "${cc}";
diff --git a/core/thread.cc b/core/thread.cc
index 54e06e7..50cc8ed 100644
--- a/core/thread.cc
+++ b/core/thread.cc
@@ -15,7 +15,7 @@
 extern "C" {
 #endif
 
-#if defined(__linux__)
+#if !defined(CONFIG_CPP_THREAD)
 
 int thread_create(thread_t *ts_p, void *(*func)(void *), void *arg)
 {
diff --git a/include/gw/thread.h b/include/gw/thread.h
index 68a0ee7..f383bd2 100644
--- a/include/gw/thread.h
+++ b/include/gw/thread.h
@@ -10,7 +10,7 @@
 extern "C" {
 #endif
 
-#if defined(__linux__)
+#if !defined(CONFIG_CPP_THREAD)
 #include <pthread.h>
 typedef pthread_t thread_t;
 typedef pthread_mutex_t mutex_t;
-- 
Alviro Iskandar Setiawan

Re: [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation

[Ammar Faizi]

On Sat, 11 Mar 2023 11:28:07 +0000, Alviro Iskandar Setiawan wrote:
> The current C++ mutex implementation is undefined behavior, specifically
> in the cond_wait() function because std::unique_lock<std::mutex> is
> constructed with std::defer_lock while the same thread has acquired the
> lock. Also, right after that defer_lock, std::condition_variable calls
> wait() with a unique lock, not in a locked state.
> 
> In such a situation, the correct construction is using std::adopt_lock.
> However, using std::adopt_lock leads to another issue. The issue is the
> lock will be released upon return in the cond_wait(). To solve the
> problem, introduce a new helper function, __cond_wait() which will
> release the lock with std::adopt_lock and then call it from cond_wait().
> The cond_wait() then acquires the lock again before it returns. The
> result is that we correctly fulfill the __must_hold() semantic while
> conforming to the C++ mutex implementation.
> 
> [...]

Applied, thanks!

[1/3] MAINTAINERS: Add myself as the thread maintainer
      commit: 5be6c427164522be650985c93595615c4f5c00e2
[2/3] core/thread: Fix undefined behavior in the C++ mutex implementation
      commit: bd84193e908c455c7c0c7f94a914d46b75557815
[3/3] configure: Introduce `--cpp-thread` option
      commit: 45b140357906d810f023ed1735a2b6585f4a5dce

Best regards,
-- 
Ammar Faizi