[vlc-devel] [PATCH 2/2] win32: fix use-after-free at thread end

[Steve Lhomme]

On 2020-09-07 10:33, Rémi Denis-Courmont wrote:
> Hi,
> 
> Thanks for the review. I don't think the potential bug applies to 
> plugins: Plugin module instances must already join their threads before 
> they are destroyed. This should ensure that the thread code is already 
> back from plugin code to LibVLC's vlc_entry(). So it is safe to unload 
> plugins already now. If not, then I'd say that's a separate bug that 
> I'll look at separately.
> 
> I have however noticed that this patch is wrong because it mismatches 
> _beginthreadex() with ExitThread().
> 
> We have two alternatives, I think:
> 1) Switch to CreateThread() like you already did on Windows Store.

The documentation for CreateThread says otherwise:

"A thread in an executable that calls the C run-time library (CRT) 
should use the _beginthreadex and _endthreadex functions for thread 
management rather than CreateThread and ExitThread; this requires the 
use of the multithreaded version of the CRT."

In UWP these APIs are not available (on win8) [1]

In the context of Universal CRT I'm not sure how this applies. I suppose 
the UCRT is always multithread aware. But we do you the CRT from pretty 
much all the threads we create, so we should probably stick to 
_beginthreadex(). Winstore 8.x support should probably removed anyway.

[1] 
https://docs.microsoft.com/en-us/cpp/cppcx/crt-functions-not-supported-in-universal-windows-platform-apps

> 2) Assume that _endthreadex() calls FreeLibraryAndExitThread() 
> internally (?) and drop this patch entirely.
> 
> Le 7 septembre 2020 08:51:13 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> 
> a écrit :
> 
>     On 2020-09-06 19:12, Rémi Denis-Courmont wrote:
> 
>         Use FreeLibraryAndExitThread() instead of ExitThread() as Win32 does
>         not natively support non-detached threads. This fixes a race
>         condition
>         when LibVLC is unloaded (see also MSDN on
>         FreeLibraryAndExitThread()).
> 
> 
>     That sounds better indeed.
>     It may be worth noting that it doesn't apply to DLLs from modules or
>     even libvlc, only the libvlccore DLL (where vlc_entry resides).
> 
>         ------------------------------------------------------------------------
>         src/win32/thread.c | 18 +++++++++++++-----
>         1 file changed, 13 insertions(+), 5 deletions(-)
> 
>         diff --git a/src/win32/thread.c b/src/win32/thread.c
>         index cd15828128..e7f2ee27d3 100644
>         --- a/src/win32/thread.c
>         +++ b/src/win32/thread.c
>         @@ -56,6 +56,7 @@ static DWORD thread_key;
>         struct vlc_thread
>         {
>         HANDLE id;
>         + HMODULE lib;
> 
>         bool killable;
>         atomic_bool killed;
>         @@ -348,14 +349,12 @@ static void
>         vlc_thread_destroy(vlc_thread_t th)
> 
>         static noreturn void vlc_thread_exit(vlc_thread_t self)
>         {
>         + HMODULE lib = self->lib;
>         +
>         if (self->id == NULL) /* Detached thread */
>         vlc_thread_destroy(self);
> 
>         -#if VLC_WINSTORE_APP
>         - ExitThread(0);
>         -#else // !VLC_WINSTORE_APP
>         - _endthreadex(0);
>         -#endif // !VLC_WINSTORE_APP
>         + FreeLibraryAndExitThread(lib, 0);
>         }
> 
>         static
>         @@ -381,6 +380,14 @@ static int vlc_clone_attr (vlc_thread_t
>         *p_handle, bool detached,
>         struct vlc_thread *th = malloc (sizeof (*th));
>         if (unlikely(th == NULL))
>         return ENOMEM;
>         +
>         + if (!GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
>         + (const void *)vlc_entry, &th->lib))
>         + {
>         + free(th);
>         + return ENOBUFS;
>         + }
>         +
>         th->entry = entry;
>         th->data = data;
>         th->killable = false; /* not until vlc_entry() ! */
>         @@ -402,6 +409,7 @@ static int vlc_clone_attr (vlc_thread_t
>         *p_handle, bool detached,
>         if (h == 0)
>         {
>         int err = errno;
>         + FreeLibrary(th->lib);
>         free (th);
>         return err;
>         }
>         -- 
>         2.28.0
>         ------------------------------------------------------------------------
>         vlc-devel mailing list
>         To unsubscribe or modify your subscription options:
>         https://mailman.videolan.org/listinfo/vlc-devel
> 
>     ------------------------------------------------------------------------
>     vlc-devel mailing list
>     To unsubscribe or modify your subscription options:
>     https://mailman.videolan.org/listinfo/vlc-devel
> 
> 
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser 
> ma brièveté.
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>