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

[Rémi Denis-Courmont]

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.
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é.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200907/9ab3c436/attachment.html>