Yesterday I did:
> * lib/windows-once.h (glwthread_once_t): Add field 'num_threads'.
> (GLWTHREAD_ONCE_INIT): Initialize it to zero.
> * lib/windows-once.c (glwthread_once): Increment num_threads while the
> thread uses the lock. Let the last thread that uses the lock destroy it.
But there is a race: If two threads are HERE at the same time:
if (once_control->inited <= 0)
{
// ---------- HERE -------------
InterlockedIncrement (&once_control->num_threads);
and one progresses until the end of the function, and the other one
then progresses until the end of the function, each of them will call
DeleteCriticalSection. I don't think this is allowed (although it has
not caused crashes in my testing on Windows 10).
These two patches fix it.
2024-05-31 Bruno Haible <br...@clisp.org>
windows-once: Fix race (regression yesterday).
* lib/windows-once.h (glwthread_once_t): Change type of inited to LONG.
* lib/windows-once.c (glwthread_once): Increment inited from 1 to 2, to
ensure that DeleteCriticalSection gets invoked only once.
windows-once: Simplify.
* lib/windows-once.c (glwthread_once): Use InterlockedCompareExchange
instead of InterlockedIncrement.
>From ea544cf5157d16540f6e4356d3865f7edea79f08 Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 31 May 2024 18:45:20 +0200
Subject: [PATCH 1/2] windows-once: Simplify.
* lib/windows-once.c (glwthread_once): Use InterlockedCompareExchange
instead of InterlockedIncrement.
---
ChangeLog | 6 ++++++
lib/windows-once.c | 5 ++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 2ebc8a792b..eff0fcfc4d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-05-31 Bruno Haible <br...@clisp.org>
+
+ windows-once: Simplify.
+ * lib/windows-once.c (glwthread_once): Use InterlockedCompareExchange
+ instead of InterlockedIncrement.
+
2024-05-31 Bruno Haible <br...@clisp.org>
pthread-once: Fix race in Cygwin workaround implementation.
diff --git a/lib/windows-once.c b/lib/windows-once.c
index b6e1c77566..75525ab197 100644
--- a/lib/windows-once.c
+++ b/lib/windows-once.c
@@ -30,7 +30,8 @@ glwthread_once (glwthread_once_t *once_control, void (*initfunction) (void))
if (once_control->inited <= 0)
{
InterlockedIncrement (&once_control->num_threads);
- if (InterlockedIncrement (&once_control->started) == 0)
+ /* If once_control->started is == -1, set it to 0. */
+ if (InterlockedCompareExchange (&once_control->started, 0, -1) < 0)
{
/* This thread is the first one to come to this once_control. */
InitializeCriticalSection (&once_control->lock);
@@ -42,8 +43,6 @@ glwthread_once (glwthread_once_t *once_control, void (*initfunction) (void))
}
else
{
- /* Don't let once_control->started grow and wrap around. */
- InterlockedDecrement (&once_control->started);
/* Some other thread has already started the initialization.
Yield the CPU while waiting for the other thread to finish
initializing and taking the lock. */
--
2.34.1
>From 454be6a1236abadc64b5451a7547a5d572e802b3 Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 31 May 2024 19:01:24 +0200
Subject: [PATCH 2/2] windows-once: Fix race (regression yesterday).
* lib/windows-once.h (glwthread_once_t): Change type of inited to LONG.
* lib/windows-once.c (glwthread_once): Increment inited from 1 to 2, to
ensure that DeleteCriticalSection gets invoked only once.
---
ChangeLog | 5 +++++
lib/windows-once.c | 39 +++++++++++++++++++++++++++++++++++++--
lib/windows-once.h | 2 +-
3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index eff0fcfc4d..a6e8855464 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
2024-05-31 Bruno Haible <br...@clisp.org>
+ windows-once: Fix race (regression yesterday).
+ * lib/windows-once.h (glwthread_once_t): Change type of inited to LONG.
+ * lib/windows-once.c (glwthread_once): Increment inited from 1 to 2, to
+ ensure that DeleteCriticalSection gets invoked only once.
+
windows-once: Simplify.
* lib/windows-once.c (glwthread_once): Use InterlockedCompareExchange
instead of InterlockedIncrement.
diff --git a/lib/windows-once.c b/lib/windows-once.c
index 75525ab197..4d234e0bf2 100644
--- a/lib/windows-once.c
+++ b/lib/windows-once.c
@@ -60,9 +60,44 @@ glwthread_once (glwthread_once_t *once_control, void (*initfunction) (void))
}
/* Here once_control->inited > 0. */
if (InterlockedDecrement (&once_control->num_threads) == 0)
- /* once_control->num_threads is now zero, and once_control->inited is 1.
+ /* once_control->num_threads is now zero, and once_control->inited > 0.
No other thread will need to use the lock.
We can therefore destroy the lock, to free resources. */
- DeleteCriticalSection (&once_control->lock);
+ /* If once_control->inited is == 1, set it to 2. */
+ if (InterlockedCompareExchange (&once_control->inited, 2, 1) == 1)
+ DeleteCriticalSection (&once_control->lock);
}
+ /* Proof of correctness:
+ * num_threads is incremented and then decremented by some threads.
+ Therefore, num_threads always stays >= 0, and is == 0 at the end.
+ * The first thread to go through the once_control->started fence
+ initializes the lock and moves inited from <= 0 to > 0. The other
+ threads don't move inited from <= 0 to > 0.
+ * inited, once > 0, stays > 0 (since at the place where it is assigned 0,
+ it cannot be > 0).
+ * inited does not change any more once it is 2.
+ Therefore, it can be changed from 1 to 2 only once.
+ * DeleteCriticalSection gets invoked right after inited has been changed
+ from 1 to 2. Therefore, DeleteCriticalSection gets invoked only once.
+ * After a moment where num_threads was 0 and inited was > 0, no thread can
+ reach an InitializeCriticalSection or EnterCriticalSection invocation.
+ Proof:
+ - At such a moment, no thread is in the code range between
+ InterlockedIncrement (&once_control->num_threads)
+ and
+ InterlockedDecrement (&once_control->num_threads)
+ - After such a moment, some thread can increment num_threads, but from
+ there they cannot reach the InitializeCriticalSection invocation,
+ because the once_control->started test prevents that, and they cannot
+ reach the EnterCriticalSection invocation in the other branch because
+ the
+ if (once_control->inited <= 0)
+ test prevents that.
+ * From this it follows that:
+ - DeleteCriticalSection cannot be executed while the lock is taken
+ (because DeleteCriticalSection is only executed after a moment where
+ num_threads was 0 and inited was > 0).
+ - Once DeleteCriticalSection has been executed, the lock is not used any
+ more.
+ */
}
diff --git a/lib/windows-once.h b/lib/windows-once.h
index 13579b5807..18ed8d878a 100644
--- a/lib/windows-once.h
+++ b/lib/windows-once.h
@@ -25,7 +25,7 @@
typedef struct
{
- volatile int inited;
+ volatile LONG inited;
volatile LONG num_threads;
volatile LONG started;
CRITICAL_SECTION lock;
--
2.34.1
