X11 unsoundness when using minifb with threads

[konnorandrews]

In the course of helping @Egggg with a SIGSEGV: invalid memory reference, I generated the following backtrace with llvm's address sanitizer.

...
==17541==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55588bdc8f70 bp 0x7f69f1968560 sp 0x7f69f1967d18 T7)
...
    #0 0x55588bdc8f70 in __sanitizer::internal_strlen(char const*) /rustc/llvm/src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_libc.cpp:167:10
    #1 0x55588bda2af1 in __interceptor_strdup /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:450:17
    #2 0x7f69e65c6858  (/usr/lib64/libX11.so.6+0x54858)
    #3 0x7f69e65c6b6c  (/usr/lib64/libX11.so.6+0x54b6c)
    #4 0x7f69e65c7307 in _XlcCreateLocaleDataBase (/usr/lib64/libX11.so.6+0x55307)
    #5 0x7f69e65cb4cf  (/usr/lib64/libX11.so.6+0x594cf)
    #6 0x7f69e65caa42  (/usr/lib64/libX11.so.6+0x58a42)
    #7 0x7f69e65cb278 in _XlcCreateLC (/usr/lib64/libX11.so.6+0x59278)
    #8 0x7f69e65ec760 in _XlcUtf8Loader (/usr/lib64/libX11.so.6+0x7a760)
    #9 0x7f69e65d295d in _XOpenLC (/usr/lib64/libX11.so.6+0x6095d)
    #10 0x7f69e65d2ada in _XrmInitParseInfo (/usr/lib64/libX11.so.6+0x60ada)
    #11 0x7f69e65ba424  (/usr/lib64/libX11.so.6+0x48424)
    #12 0x7f69e65bdb1e in XrmGetStringDatabase (/usr/lib64/libX11.so.6+0x4bb1e)
    #13 0x7f69e659a093  (/usr/lib64/libX11.so.6+0x28093)
    #14 0x7f69e659a2f5 in XGetDefault (/usr/lib64/libX11.so.6+0x282f5)
    #15 0x7f69f654f212 in _XcursorGetDisplayInfo (/usr/lib64/libXcursor.so.1+0x5212)
    #16 0x7f69f654f328 in XcursorGetDefaultSize (/usr/lib64/libXcursor.so.1+0x5328)
    #17 0x7f69f6551815 in XcursorLibraryLoadCursor (/usr/lib64/libXcursor.so.1+0x7815)
    #18 0x55588bea37f9 in minifb::os::posix::x11::DisplayInfo::load_cursor::he0d374523fbba6d7 /home/<username>/.cargo/registry/src/github.com-1ecc6299db9ec823/minifb-0.23.0/src/os/posix/x11.rs:258:18
    #19 0x55588bea31e4 in minifb::os::posix::x11::DisplayInfo::init_cursors::hd98ce9731dc012ad /home/<username>/.cargo/registry/src/github.com-1ecc6299db9ec823/minifb-0.23.0/src/os/posix/x11.rs:245:27
    #20 0x55588be9ef78 in minifb::os::posix::x11::DisplayInfo::new::h13afc602d291ed56 /home/<username>/.cargo/registry/src/github.com-1ecc6299db9ec823/minifb-0.23.0/src/os/posix/x11.rs:114:9
    #21 0x55588bea5475 in minifb::os::posix::x11::Window::new::hf38318783718700e /home/<username>/.cargo/registry/src/github.com-1ecc6299db9ec823/minifb-0.23.0/src/os/posix/x11.rs:351:21
    #22 0x55588bf51450 in minifb::os::posix::Window::new::h589266c9654934f2 /home/<username>/.cargo/registry/src/github.com-1ecc6299db9ec823/minifb-0.23.0/src/os/posix/mod.rs:47:42
    #23 0x55588bfc1306 in minifb::Window::new::hcb0be08bd6ce344a /home/<username>/.cargo/registry/src/github.com-1ecc6299db9ec823/minifb-0.23.0/src/lib.rs:247:9
...<stack trace continues up>...

After some investigation I found that minifb does not appear to run XInitThreads when it initializes it's X11 instance. Because of this, when used by multiple threads minifb calls into X11 code without proper thread synchronization.

The following is a minimal reproducible example. The error should be seen if run with RUSTFLAGS=-Zsanitizer=address cargo +nightly run.

use minifb::{Window, WindowOptions};
use std::thread::*;
use std::time::Duration;

fn main() {
    for _ in 0..10 {
        spawn(|| {
            let _ = Window::new("test", 100, 100, WindowOptions::default()).unwrap();
        });
    }
    sleep(Duration::from_secs_f32(10.0));
}

Adding the following code before calling Window::new results in address sanitizer not reporting any memory violations, and the code working as expected

let xlib = xlib::Xlib::open().unwrap();
if unsafe { (xlib.XInitThreads)() } == 0 {
     panic!("failed to init X11 threads")
}

[emoon]

Thanks for the report. It should be noted that on some OSes (such as macOS) creating windows on anything but the main thread is not supported (this is enforced by the macOS APIs and nothing minifb can do anything about)

As minifb aims to behave the same across all platforms I think it would be better simply disallow window creation on anything, but the main thread.

[emoon]

I decided to implement your suggestion. Thanks! It's included in version 0.24 that has been published