[vlc-devel] [PATCH 7/7] win32/window: remove mutex, fix heap corruption

[Steve Lhomme]

On 2020-02-03 22:13, Rémi Denis-Courmont wrote:
> A lock is overkill to swap a variable: use an atomic variable.
> Also fix a corner case double free (and racy memory read) at exit,
> by freeing the outstanding title only after the thread exits.

It would be even easier to pass a string pointer to WM_VLC_CHANGE_TEXT 
and release it when the message is handled.

While _Atomic is technically C11, it was only added to ICC in 2017 and 
there might be compilers with even less support. We currently don't use 
it anywhere in the code.

> ---
>   modules/video_output/win32/window.c | 21 +++++----------------
>   1 file changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/modules/video_output/win32/window.c b/modules/video_output/win32/window.c
> index f2d85e8c0b..2291a323af 100644
> --- a/modules/video_output/win32/window.c
> +++ b/modules/video_output/win32/window.c
> @@ -28,6 +28,7 @@
>   # include <config.h>
>   #endif
>   
> +#include <stdatomic.h>
>   #include <stdarg.h>
>   
>   #include <vlc_common.h>
> @@ -50,7 +51,6 @@
>   typedef struct vout_window_sys_t
>   {
>       vlc_thread_t thread;
> -    vlc_mutex_t  lock;
>       vlc_sem_t    ready;
>   
>       HWND hwnd;
> @@ -73,7 +73,7 @@ typedef struct vout_window_sys_t
>       LONG            i_window_style;
>   
>       /* Title */
> -    wchar_t *pwz_title;
> +    wchar_t *_Atomic pwz_title;
>   } vout_window_sys_t;
>   
>   
> @@ -146,11 +146,7 @@ static void SetTitle(vout_window_t *wnd, const char *title)
>       if (unlikely(pwz_title == NULL))
>           return;
>   
> -    vlc_mutex_lock( &sys->lock );
> -    free( sys->pwz_title );
> -    sys->pwz_title = pwz_title;
> -    vlc_mutex_unlock( &sys->lock );
> -
> +    free(atomic_exchange(&sys->pwz_title, pwz_title));
>       PostMessage( sys->hwnd, WM_VLC_CHANGE_TEXT, 0, 0 );
>   }
>   
> @@ -453,12 +449,7 @@ static long FAR PASCAL WinVoutEventProc( HWND hwnd, UINT message,
>       case WM_VLC_CHANGE_TEXT:
>           {
>               vout_window_sys_t *sys = wnd->sys;
> -            wchar_t *pwz_title;
> -
> -            vlc_mutex_lock( &sys->lock );
> -            pwz_title = sys->pwz_title;
> -            sys->pwz_title = NULL;
> -            vlc_mutex_unlock( &sys->lock );
> +            wchar_t *pwz_title = atomic_exchange(&sys->pwz_title, NULL);
>   
>               if( pwz_title )
>               {
> @@ -476,13 +467,12 @@ static void Close(vout_window_t *wnd)
>   {
>       vout_window_sys_t *sys = wnd->sys;
>   
> -    free( sys->pwz_title );
>       if (sys->hwnd)
>           PostMessage( sys->hwnd, WM_CLOSE, 0, 0 );
>   
>       vlc_join(sys->thread, NULL);
>       vlc_sem_destroy( &sys->ready );
> -    vlc_mutex_destroy( &sys->lock );
> +    free(atomic_load(&sys->pwz_title));
>   
>       if (sys->hwnd)
>           DestroyWindow( sys->hwnd );
> @@ -729,7 +719,6 @@ static int Open(vout_window_t *wnd)
>           msg_Err( wnd, "RegisterClass FAILED (err=%lu)", GetLastError() );
>           return VLC_EGENERIC;
>       }
> -    vlc_mutex_init( &sys->lock );
>       vlc_sem_init( &sys->ready, 0 );
>   
>       wnd->sys = sys;
> -- 
> 2.25.0
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>