webui-dev / webui

Use any web browser or WebView as GUI, with your preferred language in the backend and modern web technologies in the frontend, all in a lightweight portable library.
https://webui.me
MIT License
3.05k stars 184 forks source link

data race when memory alloc/free (ws non-blocking mode) #507

Open jimying opened 1 week ago

jimying commented 1 week ago

In ws non-blocking mode (win->ws_block = false), WEBUI_WS_DATA will process in new thread (_webui_ws_process_thread), which call _webui_free_mem() to free memory. If muti-threads free and main thread _webui_malloc() to alloc memory, maybe cause data race error (may crash).

we should add a mutex lock.

AlbertShown commented 1 week ago

Theoretically, the main thread creates a new block of memory each time, so each thread will receive a unique memory block address. But I see you point, those threads will free the memory blocks when the job is done, and at the same time the main thread can look for a free memory block to allocate. These actions are not synchronized in this scenario.

GCC Forums: if I tell the compiler I will be using threads (-pthreads), it links with a thread-safe version of the runtime library, including a thread-safe malloc() / free().

So, since webui in Linux is always compiled with -pthreads then malloc() and free() will use their own thread safety mechanism to avoid any data race issues.

However, adding mutex to webui malloc() and free() will either do nothing, or may avoid a data race scenario that we did not catch yet... and since webui already uses the same custom function to allocate and free, so I guess it adding mutex to it will be easy.

Something like that:

static void _webui_free_mem(void * ptr) {

    _webui_mutex_lock(&_webui.mutex_mem);
    // Job...
    _webui_mutex_unlock(&_webui.mutex_mem);
}

static void * _webui_malloc(size_t size) {

    _webui_mutex_lock(&_webui.mutex_mem);
    // Job...
    _webui_mutex_unlock(&_webui.mutex_mem);
}

So, yeah, I guess it will be better to add mutex anyway 🙂👍 Hopefully someone will propose a PR soon...