Make WM_report/WM_timer thread safe (or forbid their usage from non-main thread, and add dedicated API for such usages). #112537

New Issue

Bastien Montagne · 2023-09-18T18:39:12+02:00

Bastien Montagne commented

2023-09-18 18:39:12 +02:00

Essentially, addressing #105369 was only covering the tip of the iceberg - the whole iceberg being that the whole handling of WM_report (in therefore WM_timer) is totally not thread-safe.

This leads to invalid memory accesses (use after free and the like) when flooding reports from worker threads. Both issues described below have been encountered while trying to export a massive production file (040_0050.anim.blend from r3055) to USD, which generates a lot of reports.

use-after-free of wmTimer:

Lost the backtrace...
But what was happening was a use-after-free error in WM_event_timer_remove(), when calling WM_event_timer_free_data. The issue here was likely that that timer had been freed by the main thread after being tagged, but before the call to WM_event_timer_free_data?

Use-after-free when removing timers

9  __asan::__asan_report_store8                      asan_rtl.cpp                127  0x7fd8e6ce2d1f 
10 BLI_remlink                                       listbase.cc                 142  0x13527f0b     
11 wm_window_timers_delete_removed                   wm_window.cc                2016 0x13cf9229     
12 wm_window_timers_process                          wm_window.cc                1681 0x13cf6f27     
13 wm_window_events_process                          wm_window.cc                1700 0x13cf70f1     
14 WM_main                                           wm.cc                       606  0x13ba1c86     
15 main                                              creator.cc                  590  0xf3c332f

This is likely caused by another thread adding a report (and therefore a timer) while 'to-be-freed' timers are being deleted from main thread.

TL; DR:

We cannot keep allowing usage of WM_report API from non-main threads as-is. The code is simply not thread-safe.

Proposal

Poison usage of WM_report and WM_timer from non-main thread (add asserts in main entry points).
'low-level' threaded code is responsible to use their own ReportList data, and ensure it's properly synced with the WM one from main thread only.
Extend the wmJob API to provide easy-to-use handling of reports (each job would get their private reports list, and job update code would be responsible to ensure proper update with the WM one).

Note that the other 'obvious' solution would be to make the whole WM_report/WM_timer code thread-safe. While possible (probably even relatively easy) to do, think it's better to follow the good pattern of each thread working on its own data as much as possible. It makes code having to be aware of synchronization issues smaller (the sync code between worker and main thread, instead of a whole set of API functions). And although this is not a main concern here, it's also usually better for performances (much less locking needed).

Essentially, addressing #105369 was only covering the tip of the iceberg - the whole iceberg being that the whole handling of WM_report (in therefore WM_timer) is totally not thread-safe. This leads to invalid memory accesses (use after free and the like) when flooding reports from worker threads. Both issues described below have been encountered while trying to export a massive production file (`040_0050.anim.blend` from `r3055`) to USD, which generates _a lot_ of reports. ### use-after-free of wmTimer: *Lost the backtrace...* But what was happening was a use-after-free error in `WM_event_timer_remove()`, when calling `WM_event_timer_free_data`. The issue here was likely that that timer had been freed by the main thread after being tagged, but before the call to `WM_event_timer_free_data`? ### Use-after-free when removing timers ``` 9 __asan::__asan_report_store8 asan_rtl.cpp 127 0x7fd8e6ce2d1f 10 BLI_remlink listbase.cc 142 0x13527f0b 11 wm_window_timers_delete_removed wm_window.cc 2016 0x13cf9229 12 wm_window_timers_process wm_window.cc 1681 0x13cf6f27 13 wm_window_events_process wm_window.cc 1700 0x13cf70f1 14 WM_main wm.cc 606 0x13ba1c86 15 main creator.cc 590 0xf3c332f ``` This is likely caused by another thread adding a report (and therefore a timer) while 'to-be-freed' timers are being deleted from main thread. # TL; DR: We cannot keep allowing usage of `WM_report` API from non-main threads as-is. The code is simply _not_ thread-safe. # Proposal 1. Poison usage of `WM_report` and `WM_timer` from non-main thread (add asserts in main entry points). 2. 'low-level' threaded code is responsible to use their own ReportList data, and ensure it's properly synced with the WM one from main thread only. 3. Extend the wmJob API to provide easy-to-use handling of reports (each job would get their private reports list, and job update code would be responsible to ensure proper update with the WM one). Note that the other 'obvious' solution would be to make the whole WM_report/WM_timer code thread-safe. While possible (probably even relatively easy) to do, think it's better to follow the good pattern of each thread working on its own data as much as possible. It makes code having to be aware of synchronization issues smaller (the sync code between worker and main thread, instead of a whole set of API functions). And although this is not a main concern here, it's also usually better for performances (much less locking needed).

Bastien Montagne added the

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Make WM_report/WM_timer thread safe (or forbid their usage from non-main thread, and add dedicated API for such usages). #112537

use-after-free of wmTimer:

Use-after-free when removing timers

TL; DR:

Proposal