Skip to content

Commit 5ca419e

Browse files
authored
Merge pull request #94801 from gamelessone/fix-cond-var
Fix use-after-free of `ConditionVariable` in `ResourceLoader`
2 parents 3b3d622 + 2ff6594 commit 5ca419e

File tree

2 files changed

+14
-8
lines changed

2 files changed

+14
-8
lines changed

core/io/resource_loader.cpp

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -340,11 +340,10 @@ void ResourceLoader::_thread_load_function(void *p_userdata) {
340340
load_task.status = THREAD_LOAD_LOADED;
341341
}
342342

343-
if (load_task.cond_var) {
343+
if (load_task.cond_var && load_task.need_wait) {
344344
load_task.cond_var->notify_all();
345-
memdelete(load_task.cond_var);
346-
load_task.cond_var = nullptr;
347345
}
346+
load_task.need_wait = false;
348347

349348
bool ignoring = load_task.cache_mode == ResourceFormatLoader::CACHE_MODE_IGNORE || load_task.cache_mode == ResourceFormatLoader::CACHE_MODE_IGNORE_DEEP;
350349
bool replacing = load_task.cache_mode == ResourceFormatLoader::CACHE_MODE_REPLACE || load_task.cache_mode == ResourceFormatLoader::CACHE_MODE_REPLACE_DEEP;
@@ -723,15 +722,21 @@ Ref<Resource> ResourceLoader::_load_complete_inner(LoadToken &p_load_token, Erro
723722
DEV_ASSERT(wtp_task_err == OK);
724723
thread_load_mutex.lock();
725724
}
726-
} else {
725+
} else if (load_task.need_wait) {
727726
// Loading thread is main or user thread.
728727
if (!load_task.cond_var) {
729728
load_task.cond_var = memnew(ConditionVariable);
730729
}
730+
load_task.awaiters_count++;
731731
do {
732732
load_task.cond_var->wait(p_thread_load_lock);
733733
DEV_ASSERT(thread_load_tasks.has(p_load_token.local_path) && p_load_token.get_reference_count());
734-
} while (load_task.cond_var);
734+
} while (load_task.need_wait);
735+
load_task.awaiters_count--;
736+
if (load_task.awaiters_count == 0) {
737+
memdelete(load_task.cond_var);
738+
load_task.cond_var = nullptr;
739+
}
735740
}
736741
} else {
737742
if (loader_is_wtp) {
@@ -1159,11 +1164,10 @@ void ResourceLoader::clear_thread_load_tasks() {
11591164
if (thread_load_tasks.size()) {
11601165
for (KeyValue<String, ResourceLoader::ThreadLoadTask> &E : thread_load_tasks) {
11611166
if (E.value.status == THREAD_LOAD_IN_PROGRESS) {
1162-
if (E.value.cond_var) {
1167+
if (E.value.cond_var && E.value.need_wait) {
11631168
E.value.cond_var->notify_all();
1164-
memdelete(E.value.cond_var);
1165-
E.value.cond_var = nullptr;
11661169
}
1170+
E.value.need_wait = false;
11671171
none_running = false;
11681172
}
11691173
}

core/io/resource_loader.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,8 @@ class ResourceLoader {
167167
Thread::ID thread_id = 0; // Used if running on an user thread (e.g., simple non-threaded load).
168168
bool awaited = false; // If it's in the pool, this helps not awaiting from more than one dependent thread.
169169
ConditionVariable *cond_var = nullptr; // In not in the worker pool or already awaiting, this is used as a secondary awaiting mechanism.
170+
uint32_t awaiters_count = 0;
171+
bool need_wait = true;
170172
LoadToken *load_token = nullptr;
171173
String local_path;
172174
String remapped_path;

0 commit comments

Comments
 (0)