* [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation
@ 2023-03-11 11:28 Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 1/3] MAINTAINERS: Add myself as the thread maintainer Alviro Iskandar Setiawan
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Alviro Iskandar Setiawan @ 2023-03-11 11:28 UTC (permalink / raw)
To: Ammar Faizi
Cc: Alviro Iskandar Setiawan, Irvan Malik Azantha,
GNU/Weeb Mailing List
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 <[email protected]>
---
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC PATCH v1 1/3] MAINTAINERS: Add myself as the thread maintainer
2023-03-11 11:28 [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation Alviro Iskandar Setiawan
@ 2023-03-11 11:28 ` Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 2/3] core/thread: Fix undefined behavior in the C++ mutex implementation Alviro Iskandar Setiawan
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Alviro Iskandar Setiawan @ 2023-03-11 11:28 UTC (permalink / raw)
To: Ammar Faizi
Cc: Alviro Iskandar Setiawan, Irvan Malik Azantha,
GNU/Weeb Mailing List
Signed-off-by: Alviro Iskandar Setiawan <[email protected]>
---
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 <[email protected]>
+L: GNU/Weeb Mailing List <[email protected]>
+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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v1 2/3] core/thread: Fix undefined behavior in the C++ mutex implementation
2023-03-11 11:28 [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 1/3] MAINTAINERS: Add myself as the thread maintainer Alviro Iskandar Setiawan
@ 2023-03-11 11:28 ` Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 3/3] configure: Introduce `--cpp-thread` option Alviro Iskandar Setiawan
2023-03-12 19:40 ` [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation Ammar Faizi
3 siblings, 0 replies; 5+ messages in thread
From: Alviro Iskandar Setiawan @ 2023-03-11 11:28 UTC (permalink / raw)
To: Ammar Faizi
Cc: Alviro Iskandar Setiawan, Irvan Malik Azantha,
GNU/Weeb Mailing List
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v1 3/3] configure: Introduce `--cpp-thread` option
2023-03-11 11:28 [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 1/3] MAINTAINERS: Add myself as the thread maintainer Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 2/3] core/thread: Fix undefined behavior in the C++ mutex implementation Alviro Iskandar Setiawan
@ 2023-03-11 11:28 ` Alviro Iskandar Setiawan
2023-03-12 19:40 ` [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation Ammar Faizi
3 siblings, 0 replies; 5+ messages in thread
From: Alviro Iskandar Setiawan @ 2023-03-11 11:28 UTC (permalink / raw)
To: Ammar Faizi
Cc: Alviro Iskandar Setiawan, Irvan Malik Azantha,
GNU/Weeb Mailing List
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation
2023-03-11 11:28 [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation Alviro Iskandar Setiawan
` (2 preceding siblings ...)
2023-03-11 11:28 ` [RFC PATCH v1 3/3] configure: Introduce `--cpp-thread` option Alviro Iskandar Setiawan
@ 2023-03-12 19:40 ` Ammar Faizi
3 siblings, 0 replies; 5+ messages in thread
From: Ammar Faizi @ 2023-03-12 19:40 UTC (permalink / raw)
To: Alviro Iskandar Setiawan
Cc: Ammar Faizi, Irvan Malik Azantha, GNU/Weeb Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-12 19:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-11 11:28 [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 1/3] MAINTAINERS: Add myself as the thread maintainer Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 2/3] core/thread: Fix undefined behavior in the C++ mutex implementation Alviro Iskandar Setiawan
2023-03-11 11:28 ` [RFC PATCH v1 3/3] configure: Introduce `--cpp-thread` option Alviro Iskandar Setiawan
2023-03-12 19:40 ` [RFC PATCH v1 0/3] Fix undefined behavior in the C++ mutex implementation Ammar Faizi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox