Skip to content

Commit bb80b55

Browse files
GeorgOstrovskicopybara-github
authored andcommitted
Fix data race in mju_getLogConfigPtr lazy init
The lazy init of log_config used a single atomic flag (env_checked) with a load-then-store pattern, allowing two threads to both enter the init path and concurrently write to the non-atomic log_config.topics field. Replace with a two-phase atomic init: an atomic exchange on env_init_claimed ensures exactly one thread enters the init, while env_init_done (with acquire/release semantics) signals completion and provides the happens-before edge that makes log_config writes visible to other threads. Also adds mj_atomic_exchange_bool to engine_crossplatform.h (both MSVC and GCC/Clang variants). PiperOrigin-RevId: 941202181 Change-Id: I59b6801c7b4b8b0e1647d82d2aeb534a90fb66fb
1 parent 6e8a79c commit bb80b55

2 files changed

Lines changed: 31 additions & 6 deletions

File tree

src/engine/engine_crossplatform.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,20 @@
8585
__atomic_fetch_add(ptr, val, __ATOMIC_RELAXED)
8686
#endif
8787

88+
// Atomics helpers for mjtBool (1-byte _Bool) with acquire/release semantics.
89+
#if defined(_MSC_VER) && !defined(__clang__)
90+
#define mj_atomic_load_bool(ptr) \
91+
(mjtBool) _InterlockedCompareExchange8((volatile char*)(ptr), 0, 0)
92+
#define mj_atomic_store_bool(ptr, val) \
93+
(void)_InterlockedExchange8((volatile char*)(ptr), (char)(val))
94+
#define mj_atomic_exchange_bool(ptr, val) \
95+
(mjtBool) _InterlockedExchange8((volatile char*)(ptr), (char)(val))
96+
#else
97+
#define mj_atomic_load_bool(ptr) __atomic_load_n(ptr, __ATOMIC_ACQUIRE)
98+
#define mj_atomic_store_bool(ptr, val) __atomic_store_n(ptr, val, __ATOMIC_RELEASE)
99+
#define mj_atomic_exchange_bool(ptr, val) __atomic_exchange_n(ptr, val, __ATOMIC_ACQ_REL)
100+
#endif
101+
88102
#ifdef __cplusplus
89103
extern "C" {
90104
#endif

src/engine/engine_util_errmem.c

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,8 @@ static mjLogConfig log_config = {.logto_console = true,
104104
.logto_file = true,
105105
.logfile = "MUJOCO_LOG.TXT",
106106
.topics = 0};
107-
static mjtBool env_checked = 0;
107+
static mjtBool env_init_claimed = 0; // claimed by first thread to enter init
108+
static mjtBool env_init_done = 0; // set after log_config init completes
108109

109110
// parse MUJOCO_LOG_TOPICS env var to seed initial topic bitmask
110111
// example: MUJOCO_LOG_TOPICS="time_stp,sleep"
@@ -143,9 +144,17 @@ static void mju_initLogTopicsFromEnv(void) {
143144

144145
// private pointer getter encapsulates lazy init with zero copy overhead
145146
static const mjLogConfig* mju_getLogConfigPtr(void) {
146-
if (!env_checked) {
147-
mju_initLogTopicsFromEnv();
148-
env_checked = 1;
147+
if (!mj_atomic_load_bool(&env_init_done)) {
148+
// atomically claim right to initialize; only one thread gets old value 0
149+
if (!mj_atomic_exchange_bool(&env_init_claimed, 1)) {
150+
mju_initLogTopicsFromEnv();
151+
mj_atomic_store_bool(&env_init_done, 1);
152+
} else {
153+
// another thread is initializing; spin until it completes
154+
while (!mj_atomic_load_bool(&env_init_done)) {
155+
// empty
156+
}
157+
}
149158
}
150159
return &log_config;
151160
}
@@ -171,8 +180,9 @@ mjLogConfig mju_getLogConfig(void) {
171180

172181
// set default handler configuration
173182
void mju_setLogConfig(mjLogConfig config) {
174-
env_checked = 1;
175183
log_config = config;
184+
mj_atomic_store_bool(&env_init_claimed, 1);
185+
mj_atomic_store_bool(&env_init_done, 1);
176186
}
177187

178188
// restore default processing
@@ -182,8 +192,9 @@ void mju_clearHandlers(void) {
182192
.logto_file = true,
183193
.logfile = "MUJOCO_LOG.TXT",
184194
.topics = 0};
185-
env_checked = 1;
195+
mj_atomic_store_bool(&env_init_claimed, 1);
186196
mju_initLogTopicsFromEnv();
197+
mj_atomic_store_bool(&env_init_done, 1);
187198

188199
mju_user_error = 0;
189200
mju_user_warning = 0;

0 commit comments

Comments
 (0)